Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add secondary action for zoomin #2956

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

xiota
Copy link
Contributor

@xiota xiota commented Oct 26, 2021

This PR adds secondary actions for zoomin so that a second keybinding can be assigned. This allows both <primary>plus and <primary>equal to be used for zooming in, which potentially resolves #596 and #1712.

@elextr
Copy link
Member

elextr commented Oct 27, 2021

I would normally whinge about screwing with peoples muscle memory if changing default keybindings, but the only two apps I have found that still use <ctrl><shift>equals (aka <ctrl>plus) are Geany and Eclipse, so those are the ones screwing the muscle memory.

Also personally I don't use zoom so I don't care either way.

@xiota
Copy link
Contributor Author

xiota commented Oct 28, 2021

I don't use zoom either, but have a few other PRs that affect zoom behavior. I also figure 4-6 years should be long enough to wait to fix an issue that has affected more than one person (#596 and #1712).

@eht16
Copy link
Member

eht16 commented Oct 28, 2021

No because <Primary>-<Equal> would be very uncomfortable on my German keyboard layout!

Seriously, I understand the point and that the equal sign is easier to type than the plus on a US keyboard layout. Interestingly, on a German layout it's vice versa.

Regardless, what I'm wondering about and what I don't understand is that the Gnome HIGs suggest <Primary>-<Plus> (https://developer.gnome.org/hig/reference/keyboard.html?highlight=zoom#view-options). And I guess this is also where the current default value in Geany has its source.

Until this PR I wasn't aware of <Primary>-<Equal> for zoom in. But just noticed Firefox (at least in my configuration) accepts <Primary>-<Equal> and <Primary>-<Plus> for zoom in.

@xiota
Copy link
Contributor Author

xiota commented Oct 28, 2021

@eht16 Maybe Geany could accept both as well? Is there a way to have two keybindings for the same action, or would another action need to be added?

@eht16
Copy link
Member

eht16 commented Oct 28, 2021

That is what I was thinking about, to have both shortcuts by default and (probably) only one can be edited.

I guess this is not possible right now but a second hard-coded keybinding can be added easily probably. The more relevant question is - do we want this or is switching to <Primary>-<Equal> just the way to go?

@xiota
Copy link
Contributor Author

xiota commented Oct 28, 2021

If German keyboards won't work with this, it's just trading one problem for another. (We'd be changing it to something else in six years.)

I don't know how to program this, but maybe... if either <primary>plus or <primary>equal is set, the other is also accepted (are = and + on the same key on the German keyboard?). But if neither is set (the user explicitly set a different keybinding), then neither are accepted.

@elextr
Copy link
Member

elextr commented Oct 28, 2021

I guess nobody else (chrome, firefox etc) read the Gnome HIG.

Probably best not to add a hard coded action for any keybinding, some user is guaranteed to have a problem with it on an Elbonian keyboard [Dilbert reference].

Instead add a second keybinding action defaulting to <ctrl>equals so that it can be unset, modified etc if some user has a problem with it. I think it just needs duplicating this (plus the manual explaining why there are two zooms) 😄

@xiota
Copy link
Contributor Author

xiota commented Oct 28, 2021

Okay, I'll add a second zoom action (but not in the menu) and update this PR.

@xiota xiota changed the title Change default zoom-in keybinding to <Primary>equal Add secondary actions for cut/copy/paste/zoomin/zoomout Oct 29, 2021
@elextr
Copy link
Member

elextr commented Oct 29, 2021

Why cut copy paste? All discussion has been about zoom.

@xiota
Copy link
Contributor Author

xiota commented Oct 29, 2021

Why cut copy paste? All discussion has been about zoom.

So I can assign both ctrl+c/v and ctrl+shift+c/v to copy/paste. Since I was adding a second keybinding for zoom, figured I might as well add cut/copy/paste.

Now that I'm thinking about it... I really need three for paste... ctrl+u... because nano...

@xiota
Copy link
Contributor Author

xiota commented Oct 29, 2021

Okay, three for paste would be too much... I'll remove cut/copy/paste and put them in a plugin...

Should zoom out also have a second action or just zoom in?

@xiota xiota changed the title Add secondary actions for cut/copy/paste/zoomin/zoomout Add secondary actions for zoomin/zoomout Oct 29, 2021
@elextr
Copy link
Member

elextr commented Oct 29, 2021

Keybindings are a linear search through the list, whilst adding each additional one is immaterial, if each one isn't actively challenged then they keep adding until keyboard operation becomes slow.

Since nobody has complained about zoom out lets leave it out for now.

because nano...

Geany isn't Nano, if a nanoist wants to change their keybinding thats fine, but its not up to Geany to provide equivalences for every editor out there, thats plugin territory, see

@xiota
Copy link
Contributor Author

xiota commented Oct 29, 2021

Since nobody has complained about zoom out lets leave it out for now.

Okay. Will remove it.

thats plugin territory

Yes. I already talked myself into putting it into a plugin.

@xiota xiota changed the title Add secondary actions for zoomin/zoomout Add secondary action for zoomin Oct 29, 2021
@elextr
Copy link
Member

elextr commented Oct 29, 2021

Keybindings are a linear search through the list, whilst adding each additional one is immaterial, if each one isn't actively challenged then they keep adding until keyboard operation becomes slow.

Just a note, every key goes through the search, and its actually keys that are not keybindings that take the longest since they search the whole list exhaustively.

@xiota
Copy link
Contributor Author

xiota commented Oct 29, 2021

every key goes through the search

So each key pressed to type a sentence in the editor has to go through the keybinding search?

Couldn't it be implemented as a jump table? So it would take constant time to locate the correct response?

@elextr
Copy link
Member

elextr commented Oct 29, 2021

So each key pressed to type a sentence in the editor has to go through the keybinding search?

AFAICT yes.

Couldn't it be implemented as a jump table? So it would take constant time to locate the correct response?

There are 2^21 Unicode code points multiplied by 2^(the number of modifiers), so no.

A hash table might possibly be a solution, just "somebody" has to do it and make sure all the special cases in this are covered.

@xiota
Copy link
Contributor Author

xiota commented Oct 29, 2021

Unicode code points

Keyboards don't create unicode code points. That's what the IME is for. Only keycodes that can actually be generated need to be checked. Most keyboards have fewer than 128 keys. The modifier keys could be checked separately. So you'd have a jump plus a few comparisons to find the right response.

@xiota
Copy link
Contributor Author

xiota commented Oct 29, 2021

gdkkeysyms.h has some strange "keys" defined. A lot of them look like "synthetic" keys. (GDK_KEY_Red?)

@elextr
Copy link
Member

elextr commented Oct 29, 2021

Keyboards don't create unicode code points.

You are right, keyval is not always a code point, although often coincidentally the value happens to be the same 😄. But keyval does use at least 25 bits (see cyrillic or armenian in gdkkeysyms.h) so its worse than I thought. Also on some keyboards with "dead" keys GDK maps a multiple key sequences into one key event without IME. And IME still generates a keyevent IIUC with Unicode keyvals.

Most keyboards have fewer than 128 keys.

Not true, some non-asian keyboards have up to 4 values per keycap, see French or probably @eht16's German keyboard. And also which values? As you can see in gdkkeysyms.h keyval can cover a wide range of values. A simple table of all possible keyvalues from keyval to function address is far too big even if state is handled separately.

Thats why I suggest a hash table (or binary tree), key is combined keyval and state from the event and value is the function to call, the number of entries is related to the number of active keybindings, not the number of possible key values and the performance is constant for hash and O(ln(entries)) for tree (say 7 compares of a 64 bit value if 128 keybindings are set). Because keyvals from any one keyboard are likely to be close to sequential the combination and hash of keyval and state needs to ensure that performance does not approach linear worst case if hash is used, probably tree is safer because it doesn't have the bad worst case performance. Don't know which is easier to use/maintain.

Keyboards are a real pain, see also #1368 for another saga.

@xiota
Copy link
Contributor Author

xiota commented Oct 29, 2021

... Keyboards are a real pain...

Looks like it. Glad someone else is willing to manage it.

src/keybindings.h Outdated Show resolved Hide resolved
@elextr
Copy link
Member

elextr commented Oct 29, 2021

Not tested, but looks good.

@xiota
Copy link
Contributor Author

xiota commented Oct 29, 2021

Force push because confused branches/changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Default zoom in keybinding doesn't work in Mac OS X
3 participants