@b4n requested changes on this pull request.
> @@ -2681,6 +2681,9 @@ indent_hard_tab_width The size of a tab > character. Don't change 8 editor_ime_interaction Input method editor (IME)'s candidate 0 to new window behaviour. May be 0 (windowed) or documents 1 (inline) +scrollwheel_lines Number of lines the scrollwheel scrolls 3 immediately First: why? Second: this is NOT the current value (which is 4, or a system value). Also, as suggested in the previous sentence, Scintilla's default can depend on the system configuration (currently, it does on Windows), so it's not necessarily a great idea to force another value, at least not with a good rationale. And, why change it from 4 to 3? or is the Windows's default 3? :) > sci_scroll_columns(editor->sci, amount); return TRUE; } + else if (!event->state || (editor_prefs.scrollwheel_zoom_disable && (event->state & GDK_CONTROL_MASK))) This doesn't actually do what you think it does (in some situations). `event->state` contains all modifier states, including e.g. <kbd>NumLock</kbd> and others, so in the non-disabled case it'll fallback to Scintilla's handling if e.g. <kbd>NumLock</kbd> is on, and your code if it's not. The proper solution is to filter through `keybindings_get_modifiers()`: ```c GdkModifierType modifiers = keybindings_get_modifiers(event->state); if (modifiers & GDK_MOD1_MASK) … else if (modifiers & … ``` > sci_scroll_columns(editor->sci, amount); return TRUE; } + else if (!event->state || (editor_prefs.scrollwheel_zoom_disable && (event->state & GDK_CONTROL_MASK))) + { + /* scroll normally */ + gint lines = editor_prefs.scrollwheel_lines; + gint amount = (event->direction == GDK_SCROLL_DOWN) ? lines : (-1 * lines); ```suggestion gint amount = (event->direction == GDK_SCROLL_DOWN) ? lines : -lines; ``` > +void sci_scroll_lines(ScintillaObject *sci, gint lines) +{ + SSM(sci, SCI_LINESCROLL, 0, (uptr_t) lines); +} + + I don't think it's worth adding a new Scintilla wrapper function for the single caller, given the call is pretty simple. Just inline the equivalent in the calling code. > @@ -2681,6 +2681,9 @@ indent_hard_tab_width The size of a tab > character. Don't change 8 editor_ime_interaction Input method editor (IME)'s candidate 0 to new window behaviour. May be 0 (windowed) or documents 1 (inline) +scrollwheel_lines Number of lines the scrollwheel scrolls 3 immediately I guess I see the reason why you added this: because you need a value when transforming <kbd>Ctrl+Scroll</kbd> to a "regular" scroll event, right? And then instead of hard-coding it, you added a setting. I guess that's a fair reason, but there's sill the issues mentioned above. And is it important that <kbd>Ctrl+Scroll</kbd> scrolls, or could it just do nothing? At any rate, at least the default should be the same as the current one. I don't care as much for Windows I admit, but it's probably easy enough to mimic what Scintilla does there as well, isn't it? The semantic of the option becomes a bit hairy though, because how to conceal a dynamic system value with a user setting? I guess it could be `0` means use system default (and fallback to 4), and any other value this amount of lines or something like that… -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3835#pullrequestreview-1999370637 You are receiving this because you are subscribed to this thread. Message ID: <geany/geany/pull/3835/review/1999370...@github.com>