Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GeanyLua: Fix geany.activate() #1234

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

xiota
Copy link
Contributor

@xiota xiota commented Mar 4, 2023

Resolves #1229.

@elextr
Copy link
Member

elextr commented Mar 5, 2023

LGBI and 👍 for fixing the doc.

@Skif-off
Copy link
Contributor

Forgive my curiosity :) Why not

if (tab_idx >= 0) {
	if (tab_idx != gtk_notebook_get_current_page(NOTEBOOK)) {
		gtk_notebook_set_current_page(NOTEBOOK, tab_idx);
	}
}

?

@xiota
Copy link
Contributor Author

xiota commented Sep 29, 2024

I haven't looked at this in a while. Behavior originally intended is to "Actvate and focus" a document. The original inner conditional if (idx != ...) prevented both. Also, gtk_notebook_set_current_page most likely already performs necessary checks, so the additional check wastes processor cycles to produce incorrect behavior.

I had misunderstood the comment. The conditional needed to be corrected from tab_idx > 0 to tab_idx >= 0.

* Fix glspi_doc.c: glspi_activate()
* Remove glspi_doc.c: doc_idx_to_tab_idx()
* Fix documentation
@xiota
Copy link
Contributor Author

xiota commented Oct 13, 2024

Confirmed original issue described in #1229 and this PR fixes it. Used list-open-files.lua to test.

Force pushed to rebase. Also corrected > 0 to >= 0. I had misunderstood @Skif-off's previous comment.

@b4n Should be ready for review and merge.

@Skif-off
Copy link
Contributor

Any news?
In fact, the plugin is broken, because one of its functions is not working (maybe this is not the main function, but then what is the main function? :))).

@xiota
Copy link
Contributor Author

xiota commented Oct 24, 2024

the plugin is broken, because one of its functions is not working

Anything that needs changes to this PR?

Any news?

I'm waiting for someone to review and commit. Maybe @frlan or @elextr ?

@Skif-off
Copy link
Contributor

@xiota I meant geany.activate(), this function is still broken in upstream.

I'm waiting for someone to review and commit.

Yes, I understand, I just thought that an extra reminder in notifications would remind contributors once again about the unresolved issue. And I hope I wasn't too annoying :))

@xiota
Copy link
Contributor Author

xiota commented Oct 24, 2024

@Skif-off No problem. Might help to have additional confirmation that this PR is working as expected. You've tested it after the rebase?

@Skif-off
Copy link
Contributor

You've tested it after the rebase?

Hmm... I thought you tested it. Joke :))
I am not a contributor, so my voice has little weight, but just in case I compiled and tested it 20 minutes ago: works fime, I didn't find any problems.

@xiota
Copy link
Contributor Author

xiota commented Oct 25, 2024

my voice has little weight ... I didn't find any problems.

Thanks for testing. Helpful for you to have checked in case I miss something else.

After this is merged, please @ me if there is an issue or PR you'd like me to look at.

@Skif-off
Copy link
Contributor

@b4n, can you add this to the 2.1.0 milestone?

I do not know when the new version of Geany is scheduled to be released, there is no alternative solution and it will be sad if all forget about this PR... :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[GeanyLua] geany.activate not activating document
4 participants