Skip to content

Comments

GeanyLua: Fix geany.activate()#1234

Merged
b4n merged 1 commit intogeany:masterfrom
xiota:pr-lua-activate
Apr 10, 2025
Merged

GeanyLua: Fix geany.activate()#1234
b4n merged 1 commit intogeany:masterfrom
xiota:pr-lua-activate

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.

@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... :)

@xiota
Copy link
Contributor Author

xiota commented Jan 19, 2025

@frlan Any chance you could take a look and merge? This fixes a bug that has been affecting at least a couple users. Thanks.

@b4n
Copy link
Member

b4n commented Mar 24, 2025

So, the actual functional changes this is supposed to make is replace the buggy doc_idx_to_tab_idx() with document_get_notebook_page(), and returning idx >= 0 instead of idx > 0, right? The rest doesn't seem to try changing much to the logic, if I follow.

I don't know the plugin or Lua, but this looks (mostly) fine -- se review comments for the more questionable bits.

@b4n b4n added this to the 2.1.0 milestone Mar 24, 2025
@xiota
Copy link
Contributor Author

xiota commented Mar 24, 2025

@b4n Thank you for reviewing. I made the suggested changes.

Copy link
Member

@b4n b4n left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGBI (not tested)

* Fix glspi_doc.c: glspi_activate()
* Remove glspi_doc.c: doc_idx_to_tab_idx()
* Fix documentation
@b4n b4n force-pushed the pr-lua-activate branch from c202ea7 to ed33e40 Compare April 10, 2025 22:33
@b4n b4n merged commit 69066e8 into geany:master Apr 10, 2025
1 of 2 checks passed
@b4n
Copy link
Member

b4n commented Apr 10, 2025

I squashed the commits, and merged this, hopefully it's been tested well enough :)
Thanks!

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