@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/[email protected]>
