There was a mistake in my fix for wayland popups, as @b4n correctly noticed in https://github.com/geany/geany/pull/3011#issuecomment-1279825987. The problem was that the "Go to symbol definition" did not open the popup in correct location.
This PR contains two commits: - First fixes the problem - Second removes support for prehistoric versions of GTK
The first commit might be used separately, but I have to openly admit that the #ifdefed code is not tested, because I couldn't find any system in my reach with old enough GTK. The second commit should not be merged until GTK 3.24 is officially required for Geany. But I guess that it should not be a problem, this PR can probably wait untill then, since no one noticed the bug for a month. You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/3316
-- Commit Summary --
* Fix go to symbol definition popup location * Drop support for GTK 3.21 and older
-- File Changes --
M src/callbacks.c (2) M src/editor.c (2) M src/msgwindow.c (9) M src/notebook.c (8) M src/plugins.c (3) M src/prefs.c (21) M src/sidebar.c (8) M src/symbols.c (91) M src/ui_utils.c (26) M src/ui_utils.h (2) M src/vte.c (2)
-- Patch Links --
https://github.com/geany/geany/pull/3316.patch https://github.com/geany/geany/pull/3316.diff
@dolik-rce commented on this pull request.
- if (event && event->type == GDK_BUTTON_PRESS)
- button_event = (GdkEventButton *) event; - else - gdk_event_free(event);
BTW: This looks like a memory leak. The event object is only freed if it is triggered by keyboard. Good thing that we'll get rid of it soon :slightly_smiling_face:
@elextr commented on this pull request.
@@ -3230,14 +3230,109 @@ gboolean ui_encodings_combo_box_set_active_encoding(GtkComboBox *combo, gint enc
return FALSE; }
-void ui_menu_popup(GtkMenu* menu, GtkMenuPositionFunc func, gpointer data, guint button, guint32 activate_time) + +#if GTK_CHECK_VERSION(3,22,0) +/* positions a popup at the caret from the ScintillaObject in @p data */ +static void position_at_carret(GtkMenu *menu, gint *x, gint *y, gboolean *push_in, gpointer data) +{
"caret" has only one `r`
BTW: This looks like a memory leak. The event object is only freed if it is triggered by keyboard. Good thing that we'll get rid of it soon slightly_smiling_face
No, the next line which sets `button_event ` as data also adds `gdk_event_free` as a destructor, so its freed when the menu object is destroyed.
@dolik-rce pushed 2 commits.
714748ae290d5c18d75851f7863360c42899e487 Fix go to symbol definition popup location 5abcbf102300671677931ec0722a5923719b8598 Drop support for GTK 3.21 and older
@dolik-rce commented on this pull request.
@@ -3230,14 +3230,109 @@ gboolean ui_encodings_combo_box_set_active_encoding(GtkComboBox *combo, gint enc
return FALSE; }
-void ui_menu_popup(GtkMenu* menu, GtkMenuPositionFunc func, gpointer data, guint button, guint32 activate_time) + +#if GTK_CHECK_VERSION(3,22,0) +/* positions a popup at the caret from the ScintillaObject in @p data */ +static void position_at_carret(GtkMenu *menu, gint *x, gint *y, gboolean *push_in, gpointer data) +{
Fixed, thanks
next line which sets button_event as data also adds gdk_event_free as a destructor
Oh, right. I have totally overlooked that.
@eht16 commented on this pull request.
{
- /* Use appropriate function for menu popup: - - gtk_menu_popup_at_pointer is not available on GTK older than 3.22 - - gtk_menu_popup is deprecated and causes issues on multimonitor wayland setups */ -#if GTK_CHECK_VERSION(3,22,0) - gtk_menu_popup_at_pointer(GTK_MENU(menu), NULL); -#else - gtk_menu_popup(GTK_MENU(menu), NULL, NULL, func, data, button, activate_time); -#endif + if (!sci) + gtk_menu_popup_at_pointer(GTK_MENU(menu), NULL); + else
Since we need this special case only for the tag completion popup, we might also keep the special logic in smbols.c as before and then we can drop this helper function altogether and use `gtk_menu_popup_at_pointer()` directly.
What do you think?
@eht16 commented on this pull request.
@@ -1404,90 +1404,10 @@ static guint get_tag_class(const TMTag *tag)
}
-/* positions a popup at the caret from the ScintillaObject in @p data */ -static void goto_popup_position_func(GtkMenu *menu, gint *x, gint *y, gboolean *push_in, gpointer data)
Nice that we do not need almost all of this code anymore.
@dolik-rce commented on this pull request.
{
- /* Use appropriate function for menu popup: - - gtk_menu_popup_at_pointer is not available on GTK older than 3.22 - - gtk_menu_popup is deprecated and causes issues on multimonitor wayland setups */ -#if GTK_CHECK_VERSION(3,22,0) - gtk_menu_popup_at_pointer(GTK_MENU(menu), NULL); -#else - gtk_menu_popup(GTK_MENU(menu), NULL, NULL, func, data, button, activate_time); -#endif + if (!sci) + gtk_menu_popup_at_pointer(GTK_MENU(menu), NULL); + else
You're right. This could be a lot simpler now, that we use only reasonably modern GTK. Actually the only reason why this helper function was created was to hide that ugly `#ifdef GTK_CHECK_VERSION(3, 22, 0)` in all the places that show menu.
@dolik-rce pushed 1 commit.
bbbfda4679f6ca2680be1149f9f0fdcae83b21e1 drop ui_menu_popup function
@techee commented on this pull request.
@@ -1404,90 +1404,10 @@ static guint get_tag_class(const TMTag *tag)
}
-/* positions a popup at the caret from the ScintillaObject in @p data */ -static void goto_popup_position_func(GtkMenu *menu, gint *x, gint *y, gboolean *push_in, gpointer data)
Yes.
(But I remember being *really* impressed when Colomban came up with this code after me not knowing how to present the popup, taking all the multimonitor setup into account and invoking the popup using a keyboard.)
@b4n requested changes on this pull request.
I don't quite like the states in-between commits (although I understand their motivation), but the final one look really promising :+1:
Apart from my comment it looks good, and could be merged whenever we do depend on 3.24 for real (which I don't see any problem myself, 3.24.0 is getting pretty old by now)
- gint y = SSM(sci, SCI_POINTYFROMPOSITION, 0, pos) + line_height;
+ GdkRectangle rect = {x, y, 0, 0}; + GdkGravity rect_anchor = gtk_widget_get_direction(GTK_WIDGET(menu)) == GTK_TEXT_DIR_RTL ? GDK_GRAVITY_NORTH_EAST : GDK_GRAVITY_NORTH_WEST;
This hides the caret (and the character at that position) if there is not enough room to put the popup below the line. It's easy enough to fix by moving `line_height` to the rect's height instead of directly shifting its Y coordinate (and using SOUTH gravities). Another possible improvement would be doing something similar in the X direction, although there's no actual issue with it. However, as is, if there isn't enough room on the right, it would then popup on the left *of the caret*, not under the character at the caret. It might be a nitpick, but it looks a tiny bit off with caret in overwrite mode. Yet, I don't think the previous code supported this.
Below is a suggestion doing both (dropping the second part is fairly straightforward though if we want to):
```suggestion gint y = SSM(sci, SCI_POINTYFROMPOSITION, 0, pos); gint pos_next = sci_get_position_after(sci, pos); gint char_width = 0; if (sci_get_line_from_position(sci, pos_next) == line) char_width = SSM(sci, SCI_POINTXFROMPOSITION, 0, pos_next) - x; GdkRectangle rect = {x, y, char_width, line_height}; GdkGravity rect_anchor = gtk_widget_get_direction(GTK_WIDGET(menu)) == GTK_TEXT_DIR_RTL ? GDK_GRAVITY_SOUTH_EAST : GDK_GRAVITY_SOUTH_WEST; ```
@@ -1403,6 +1403,20 @@ static guint get_tag_class(const TMTag *tag)
return TM_ICON_STRUCT; }
+/* opens menu at caret position */ +static void show_menu_at_caret(GtkMenu* menu, ScintillaObject *sci) +{ + GdkWindow *window = gtk_widget_get_window(GTK_WIDGET(sci)); + gint pos = sci_get_current_position(sci); + gint line = sci_get_line_from_position(sci, pos); + gint line_height = SSM(sci, SCI_TEXTHEIGHT, line, 0); + gint x = SSM(sci, SCI_POINTXFROMPOSITION, 0, pos); + gint y = SSM(sci, SCI_POINTYFROMPOSITION, 0, pos) + line_height; + GdkRectangle rect = {x, y, 0, 0}; + GdkGravity rect_anchor = gtk_widget_get_direction(GTK_WIDGET(menu)) == GTK_TEXT_DIR_RTL ? GDK_GRAVITY_NORTH_EAST : GDK_GRAVITY_NORTH_WEST;
Actually [the documentation](https://docs.gtk.org/gtk3/method.Menu.popup_at_rect.html) states:
Anchors should be specified under the assumption that the text direction is left-to-right; they will be flipped horizontally automatically if the text direction is right-to-left.
So the switch depending on text direction should be removed.
- gint y = SSM(sci, SCI_POINTYFROMPOSITION, 0, pos) + line_height;
+ GdkRectangle rect = {x, y, 0, 0}; + GdkGravity rect_anchor = gtk_widget_get_direction(GTK_WIDGET(menu)) == GTK_TEXT_DIR_RTL ? GDK_GRAVITY_NORTH_EAST : GDK_GRAVITY_NORTH_WEST; + gtk_menu_popup_at_rect(GTK_MENU(menu), window, &rect, rect_anchor, GDK_GRAVITY_NORTH_WEST, NULL);
Making the whole thing like so: ```suggestion gint y = SSM(sci, SCI_POINTYFROMPOSITION, 0, pos); gint pos_next = sci_get_position_after(sci, pos); gint char_width = 0; if (sci_get_line_from_position(sci, pos_next) == line) char_width = SSM(sci, SCI_POINTXFROMPOSITION, 0, pos_next) - x; GdkRectangle rect = {x, y, char_width, line_height}; gtk_menu_popup_at_rect(GTK_MENU(menu), window, &rect, GDK_GRAVITY_SOUTH_WEST, GDK_GRAVITY_NORTH_WEST, NULL); ```
@b4n commented on this pull request.
@@ -1404,90 +1404,10 @@ static guint get_tag_class(const TMTag *tag)
}
-/* positions a popup at the caret from the ScintillaObject in @p data */ -static void goto_popup_position_func(GtkMenu *menu, gint *x, gint *y, gboolean *push_in, gpointer data)
@techee yet as you know, no amount of cleverness can compete with no code at all :slightly_smiling_face:
I don't quite like the states in-between commits (although I understand their motivation)
I can squash or rearrange the commits if you want. The commits were primarily separated to allow immediate merge, without waiting for GTK 3.24. But I believe that it is now almost certain, that next release will require it, so it is not really needed anymore.
@dolik-rce commented on this pull request.
- gint y = SSM(sci, SCI_POINTYFROMPOSITION, 0, pos) + line_height;
+ GdkRectangle rect = {x, y, 0, 0}; + GdkGravity rect_anchor = gtk_widget_get_direction(GTK_WIDGET(menu)) == GTK_TEXT_DIR_RTL ? GDK_GRAVITY_NORTH_EAST : GDK_GRAVITY_NORTH_WEST; + gtk_menu_popup_at_rect(GTK_MENU(menu), window, &rect, rect_anchor, GDK_GRAVITY_NORTH_WEST, NULL);
Thanks, applied.
@dolik-rce pushed 1 commit.
94833eaf14a3c93c70f9bab3c6d4ad63774d2adc Fix popup overlapping the carret
Apart from my comment it looks good, and could be merged whenever we do depend on 3.24 for real (which I don't see any problem myself, 3.24.0 is getting pretty old by now)
Another possibility is to keep the old code and use ifdefs to pick the implementation based on gtk 3.24 availability (but of course there's no need for this if we are going to depend on 3.24).
I think it's pretty safe to assume that the next release will depend on 3.24 and we don't need to add new code or keep copies of old code to handle <3.24
Agree, there is general agreement buried in #3178 to require 3.24, just needs "somebody"(s) to make autotools and meson require it.
Before a final review or ideally merge of https://github.com/geany/geany-plugins/pull/1201 and https://github.com/geany/geany/pull/3315 is necessary to get the pipeline ready for GTK 3.24.
@techee @b4n do we still want this in 2.0?
@techee @b4n do we still want this in 2.0?
I would say so.
This is a wayland bugfix IIUC, so yeah would be good to include in 2.0.
Yeah I think we should. There's a conflict, and I'll re-review it once more, but it probably should make it.
@b4n commented on this pull request.
- if (sci_get_line_from_position(sci, pos_next) == line)
+ char_width = SSM(sci, SCI_POINTXFROMPOSITION, 0, pos_next) - x;
As I'm a sucker for weird corner cases, this actually does not work if line wrapping is enabled and the caret is right before the wrap position (popup would be anchored to the start of the next line).
Something like this works though: ```suggestion /* if it's on the same Y (same line and not after wrapping), diff the X */ if (SSM(sci, SCI_POINTYFROMPOSITION, 0, pos_next) == y) char_width = SSM(sci, SCI_POINTXFROMPOSITION, 0, pos_next) - x; /* otherwise, if it's not at the end of the document, use the character's width * -- which would work in case above, but it feels slower (I didn't get numbers) */ else if (pos_next > pos) { gint style = SSM(sci, SCI_GETSTYLEAT, 0, pos); gchar *text = sci_get_contents_range(sci, pos, pos_next); char_width = sci_text_width(sci, style, text); g_free(text); } ```
@b4n pushed 4 commits.
c73dc3f0aedbf9b0fbc977a213dbf65b1d8a22f9 Fix go to symbol definition popup location 229e598dd2b385d39c558b00594eed20db00f684 Drop support for GTK 3.21 and older 439aacc00596611585eeaf5b28378fef2c806bcb drop ui_menu_popup function d2a64b0ae3af4acc3bad11230a1dec8c20e4bba5 Fix popup overlapping the carret
I just rebased on top of master to fix the merge conflict. I did *not* include my latest suggested change.
@techee commented on this pull request.
- if (sci_get_line_from_position(sci, pos_next) == line)
+ char_width = SSM(sci, SCI_POINTXFROMPOSITION, 0, pos_next) - x;
Maybe it was mentioned somewhere in the previous conversation but what's the point of calculating `char_width`? Couldn't the rect width just be always `1`? Clearly the height is important so the menu is below the line but with width `1` the menu pops up from the gap between characters which looks fine and not unexpected IMO.
@techee commented on this pull request.
- if (sci_get_line_from_position(sci, pos_next) == line)
+ char_width = SSM(sci, SCI_POINTXFROMPOSITION, 0, pos_next) - x;
Another possible improvement would be doing something similar in the X direction, although there's no actual issue with it. However, as is, if there isn't enough room on the right, it would then popup on the left of the caret, not under the character at the caret. It might be a nitpick, but it looks a tiny bit off with caret in overwrite mode. Yet, I don't think the previous code supported this.
OK, found it.
@techee commented on this pull request.
- if (sci_get_line_from_position(sci, pos_next) == line)
+ char_width = SSM(sci, SCI_POINTXFROMPOSITION, 0, pos_next) - x;
But really, I'd be for the "lazy" solution here, it's not worth complicating the code too much I think.
@b4n commented on this pull request.
- if (sci_get_line_from_position(sci, pos_next) == line)
+ char_width = SSM(sci, SCI_POINTXFROMPOSITION, 0, pos_next) - x;
But really, I'd be for the "lazy" solution here, it's not worth complicating the code too much I think.
Which one do you refer to as "lazy"? * current (misbehaving with edge case of wrapping) * `SCI_POINTYFROMPOSITION`-based one (not perfect for the mentioned edge case, but not bad) * `SCI_TEXTWIDTH`-base one (probably good for all cases,but possibly slower)
I agree that all the shenanigans might not be needed here, and possibly either solution is OK. I have a preference for one that doesn't bluntly misbehaves in the edge case, but it's an *very* edge case though…
@techee commented on this pull request.
- if (sci_get_line_from_position(sci, pos_next) == line)
+ char_width = SSM(sci, SCI_POINTXFROMPOSITION, 0, pos_next) - x;
Which one do you refer to as "lazy"?
Using `char_width = 0` and not calculating it.
@b4n commented on this pull request.
- if (sci_get_line_from_position(sci, pos_next) == line)
+ char_width = SSM(sci, SCI_POINTXFROMPOSITION, 0, pos_next) - x;
Ah… I feel it looks bad if cursor is in block mode (or overwrite) and the popup is placed on the left (when there isn't enough room on the right). IMO, *one* of the solutions (preferably not the first) is better. But I won't die on that hill :slightly_smiling_face:
@techee commented on this pull request.
- if (sci_get_line_from_position(sci, pos_next) == line)
+ char_width = SSM(sci, SCI_POINTXFROMPOSITION, 0, pos_next) - x;
I don't see what the problem is in this case - this is what happens when I invoke it on `i` and move the window to the right so the popup appears on the left:
<img width="482" alt="Screenshot 2023-10-10 at 12 30 03" src="https://github.com/geany/geany/assets/713965/c99ec979-a4a9-4ca4-b514-2c60d5aff7f5">
The underline cursor of the insert mode disappears when the popup shows.
I just rebased on top of master to fix the merge conflict. I did not include my latest suggested change.
And I'm afraid I just introduced another merge conflict by merging #3547...
@b4n commented on this pull request.
- if (sci_get_line_from_position(sci, pos_next) == line)
+ char_width = SSM(sci, SCI_POINTXFROMPOSITION, 0, pos_next) - x;
This is with the current state of this PR right, not with just `char_width=0`, is it?
The problem with the current version is only if you have wrapping enabled and it wraps after the next character (e.g. if wrapping wrapped right after the `i` (not even having the space after). What you'd see is the popup on the far left of the editor.
I just rebased on top of master to fix the merge conflict. I did not include my latest suggested change.
And I'm afraid I just introduced another merge conflict by merging #3547...
No worries, I have it resolved locally and it's not a big deal.
@techee commented on this pull request.
- if (sci_get_line_from_position(sci, pos_next) == line)
+ char_width = SSM(sci, SCI_POINTXFROMPOSITION, 0, pos_next) - x;
This is with the current state of this PR right, not with just char_width=0, is it?
Yes it is (to be precise, with `char_width=1`, I wasn't sure if 0-width bounding box works alright in this case).
The problem with the current version is only if you have wrapping enabled and it wraps after the next character (e.g. if wrapping wrapped right after the i (not even having the space after). What you'd see is the popup on the far left of the editor.
I understand the current version problem but I still don't see the problem with `char_width=0`.
@techee commented on this pull request.
- if (sci_get_line_from_position(sci, pos_next) == line)
+ char_width = SSM(sci, SCI_POINTXFROMPOSITION, 0, pos_next) - x;
Yes it is
To be clear, I meant the screenshot is with the `char_width=1` version.
@b4n commented on this pull request.
- if (sci_get_line_from_position(sci, pos_next) == line)
+ char_width = SSM(sci, SCI_POINTXFROMPOSITION, 0, pos_next) - x;
With `char_width=1` (and none of the extra tests), I get this:  Caret is on the last `e` (if in block mode, it is over it, in beam mode right before).
@b4n commented on this pull request.
- if (sci_get_line_from_position(sci, pos_next) == line)
+ char_width = SSM(sci, SCI_POINTXFROMPOSITION, 0, pos_next) - x;
…and for comparison, the exact same screenshot (to my freehand abilities…) with the `SCI_POINTYFROMPOSITION` branch: 
@b4n pushed 4 commits.
d906453916469769e68a2b35345813aa66737e7e Fix go to symbol definition popup location 956409479a4dda51e95357cbb28336c121724238 Drop support for GTK 3.21 and older bbfc201313f948f0528280a9089bc79af1af98a0 drop ui_menu_popup function 4a2999c18e2db59bb40b90ad6453732a2596a316 Fix popup overlapping the carret
Re-rebased on top of master to fix merge conflicts
@techee commented on this pull request.
- if (sci_get_line_from_position(sci, pos_next) == line)
+ char_width = SSM(sci, SCI_POINTXFROMPOSITION, 0, pos_next) - x;
Yeah, and what's the problem here? You would like to have the popup's right corner one character to the right? And how is it related to the shape of the caret?
Basically Scintilla internally always has caret between letters and only draws it over the next letter when for instance in a block mode (something I was fighting with in my vimode plugin because vi itself has caret on letters and sometimes this leads to some surprises in character number calculations). So caret over "e" in block mode is actually caret before "e".
But I still don't see the "wrongness" here - the popup jumps from the bottom-left corner of the block mode caret both when there is enough space on the right and also when there isn't and the popup is on the left, so nothing unexpected I'd say. Also this is a rare case where two conditions have to be met - not enough space and a block caret (and I never use overwrite mode, I just had to learn how to invoke "insert" on the mac :-).
But don't hesitate to implement it your way if this is something that bothers you :-).
@b4n pushed 1 commit.
3498fe12a3be953511b810e568991e4149f2613a Fix popup position on a wrapping corner case
@b4n commented on this pull request.
- if (sci_get_line_from_position(sci, pos_next) == line)
+ char_width = SSM(sci, SCI_POINTXFROMPOSITION, 0, pos_next) - x;
I guess I feel like the popup should be under the letter the caret is at (not sure what are the real differences between having the caret over or before a character though, offsets should be pretty much the same… but this is kind of irrelevant to the feel :))
But don't hesitate to implement it your way if this is something that bothers you :-).
You nailed it I guess :D
I went ahead and implemented the more basic version that is similar to what was there but don't produce incorrect results on the wrapping corner case. So I'm happier, and you're not too unhappy because it's not as complicated as it could be :grin:
@techee commented on this pull request.
- if (sci_get_line_from_position(sci, pos_next) == line)
+ char_width = SSM(sci, SCI_POINTXFROMPOSITION, 0, pos_next) - x;
I went ahead and implemented the more basic version that is similar to what was there but don't produce incorrect results on the wrapping corner case. So I'm happier, and you're not too unhappy because it's not as complicated as it could be
The best of both worlds indeed, LGTM :-)
@b4n pushed 3 commits.
869606ee1427b3611d01ee37c4811942241bb320 Pass the event to gtk_menu_popup_at_pointer() when possible 705dc3c8eb39c43e54daf171811b412d6eaa5032 Use gtk_menu_popup_at_pointer() in filebrowser as well d10cfb503d30ee9bb7848f171dc857e2e90f0de5 Position goto popup at the mouse when triggered with it
@b4n approved this pull request.
I added a few minor changes, including a small behavioral one: the goto popup is again positioned at the mouse if triggered with it, which makes it slightly better for that use case.
I added a few minor changes, including a small behavioral one: the goto popup is again positioned at the mouse if triggered with it, which makes it slightly better for that use case.
Looks good (and works too from what I've tried) - though the previous version was just fine too. Apparently I'm not such a popup gourmet :-)
Merged #3316 into master.
github-comments@lists.geany.org