[Github-comments] Re: [geany/geany] Fix goto popup location (PR #3316)
Merged #3316 into master. -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3316#event-10658414985 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] Fix goto popup location (PR #3316)
> 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 :-) -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3316#issuecomment-1763488969 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] Fix goto popup location (PR #3316)
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. -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3316#issuecomment-1763481385 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] Fix goto popup location (PR #3316)
@b4n approved this pull request. -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3316#pullrequestreview-1678848601 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] Fix goto popup location (PR #3316)
@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 -- View it on GitHub: https://github.com/geany/geany/pull/3316/files/3498fe12a3be953511b810e568991e4149f2613a..d10cfb503d30ee9bb7848f171dc857e2e90f0de5 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] Fix goto popup location (PR #3316)
@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 :-) -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3316#discussion_r1358313969 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] Fix goto popup location (PR #3316)
@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: -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3316#discussion_r1357412991 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] Fix goto popup location (PR #3316)
@b4n pushed 1 commit. 3498fe12a3be953511b810e568991e4149f2613a Fix popup position on a wrapping corner case -- View it on GitHub: https://github.com/geany/geany/pull/3316/files/4a2999c18e2db59bb40b90ad6453732a2596a316..3498fe12a3be953511b810e568991e4149f2613a You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] Fix goto popup location (PR #3316)
@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 :-). -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3316#discussion_r1352543579 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] Fix goto popup location (PR #3316)
Re-rebased on top of master to fix merge conflicts -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3316#issuecomment-1755442929 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] Fix goto popup location (PR #3316)
@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 -- View it on GitHub: https://github.com/geany/geany/pull/3316/files/d2a64b0ae3af4acc3bad11230a1dec8c20e4bba5..4a2999c18e2db59bb40b90ad6453732a2596a316 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] Fix goto popup location (PR #3316)
@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:  -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3316#discussion_r1352489537 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] Fix goto popup location (PR #3316)
@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). -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3316#discussion_r1352432147 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] Fix goto popup location (PR #3316)
@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. -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3316#discussion_r1352356861 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] Fix goto popup location (PR #3316)
@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`. -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3316#discussion_r1352355275 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] Fix goto popup location (PR #3316)
> > 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. -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3316#issuecomment-1755246888 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] Fix goto popup location (PR #3316)
@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. -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3316#discussion_r1352334264 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] Fix goto popup location (PR #3316)
> 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... -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3316#issuecomment-1755041680 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] Fix goto popup location (PR #3316)
@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: https://github.com/geany/geany/assets/713965/c99ec979-a4a9-4ca4-b514-2c60d5aff7f5";> The underline cursor of the insert mode disappears when the popup shows. -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3316#discussion_r1352152039 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] Fix goto popup location (PR #3316)
@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: -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3316#discussion_r1352119547 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] Fix goto popup location (PR #3316)
@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. -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3316#discussion_r1352104374 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] Fix goto popup location (PR #3316)
@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… -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3316#discussion_r1352085215 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] Fix goto popup location (PR #3316)
@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. -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3316#discussion_r1350746150 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] Fix goto popup location (PR #3316)
@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. -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3316#discussion_r1350743555 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] Fix goto popup location (PR #3316)
@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. -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3316#discussion_r1350737253 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] Fix goto popup location (PR #3316)
I just rebased on top of master to fix the merge conflict. I did *not* include my latest suggested change. -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3316#issuecomment-1752161331 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] Fix goto popup location (PR #3316)
@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 -- View it on GitHub: https://github.com/geany/geany/pull/3316/files/94833eaf14a3c93c70f9bab3c6d4ad63774d2adc..d2a64b0ae3af4acc3bad11230a1dec8c20e4bba5 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] Fix goto popup location (PR #3316)
@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);
}
```
--
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany/pull/3316#pullrequestreview-1663599640
You are receiving this because you are subscribed to this thread.
Message ID:
[Github-comments] Re: [geany/geany] Fix goto popup location (PR #3316)
Yeah I think we should. There's a conflict, and I'll re-review it once more, but it probably should make it. -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3316#issuecomment-1752145766 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] Fix goto popup location (PR #3316)
This is a wayland bugfix IIUC, so yeah would be good to include in 2.0. -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3316#issuecomment-1751978216 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] Fix goto popup location (PR #3316)
> @techee @b4n do we still want this in 2.0? I would say so. -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3316#issuecomment-1751976951 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] Fix goto popup location (PR #3316)
@techee @b4n do we still want this in 2.0? -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3316#issuecomment-1751845522 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] Fix goto popup location (PR #3316)
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. -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3316#issuecomment-1371147809 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] Fix goto popup location (PR #3316)
Agree, there is general agreement buried in #3178 to require 3.24, just needs "somebody"(s) to make autotools and meson require it. -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3316#issuecomment-1369432930 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] Fix goto popup location (PR #3316)
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 -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3316#issuecomment-1369424789 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] Fix goto popup location (PR #3316)
> 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). -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3316#issuecomment-1368549306 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] Fix goto popup location (PR #3316)
@dolik-rce pushed 1 commit. 94833eaf14a3c93c70f9bab3c6d4ad63774d2adc Fix popup overlapping the carret -- View it on GitHub: https://github.com/geany/geany/pull/3316/files/bbbfda4679f6ca2680be1149f9f0fdcae83b21e1..94833eaf14a3c93c70f9bab3c6d4ad63774d2adc You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] Fix goto popup location (PR #3316)
@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.
--
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany/pull/3316#discussion_r1059635236
You are receiving this because you are subscribed to this thread.
Message ID:
[Github-comments] Re: [geany/geany] Fix goto popup location (PR #3316)
> 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. -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3316#issuecomment-1368207980 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] Fix goto popup location (PR #3316)
@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: -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3316#discussion_r1059516255 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] Fix goto popup location (PR #3316)
@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);
```
--
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany/pull/3316#pullrequestreview-1233241854
You are receiving this because you are subscribed to this thread.
Message ID:
[Github-comments] Re: [geany/geany] Fix goto popup location (PR #3316)
@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.) -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3316#discussion_r1009924040 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] Fix goto popup location (PR #3316)
@dolik-rce pushed 1 commit. bbbfda4679f6ca2680be1149f9f0fdcae83b21e1 drop ui_menu_popup function -- View it on GitHub: https://github.com/geany/geany/pull/3316/files/5abcbf102300671677931ec0722a5923719b8598..bbbfda4679f6ca2680be1149f9f0fdcae83b21e1 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] Fix goto popup location (PR #3316)
@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.
--
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany/pull/3316#discussion_r1002734473
You are receiving this because you are subscribed to this thread.
Message ID:
[Github-comments] Re: [geany/geany] Fix goto popup location (PR #3316)
@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. -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3316#pullrequestreview-1152303758 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] Fix goto popup location (PR #3316)
@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?
--
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany/pull/3316#pullrequestreview-1152301740
You are receiving this because you are subscribed to this thread.
Message ID:
[Github-comments] Re: [geany/geany] Fix goto popup location (PR #3316)
> next line which sets button_event as data also adds gdk_event_free as a > destructor Oh, right. I have totally overlooked that. -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3316#issuecomment-1283389879 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] Fix goto popup location (PR #3316)
@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
--
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany/pull/3316#discussion_r998919612
You are receiving this because you are subscribed to this thread.
Message ID:
[Github-comments] Re: [geany/geany] Fix goto popup location (PR #3316)
@dolik-rce pushed 2 commits. 714748ae290d5c18d75851f7863360c42899e487 Fix go to symbol definition popup location 5abcbf102300671677931ec0722a5923719b8598 Drop support for GTK 3.21 and older -- View it on GitHub: https://github.com/geany/geany/pull/3316/files/3bf610ca460c309fb304ec53a5c0cb235212bca5..5abcbf102300671677931ec0722a5923719b8598 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] Fix goto popup location (PR #3316)
> 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. -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3316#issuecomment-1283159362 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] Fix goto popup location (PR #3316)
@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`
--
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany/pull/3316#pullrequestreview-1146653023
You are receiving this because you are subscribed to this thread.
Message ID:
[Github-comments] Re: [geany/geany] Fix goto popup location (PR #3316)
@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: -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3316#pullrequestreview-1146296604 You are receiving this because you are subscribed to this thread. Message ID:
