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

duplicated entries in shortcuts conf #132

Closed
agaida opened this issue Aug 22, 2019 · 19 comments
Closed

duplicated entries in shortcuts conf #132

agaida opened this issue Aug 22, 2019 · 19 comments
Assignees
Projects

Comments

@agaida
Copy link
Member

agaida commented Aug 22, 2019

Expected Behavior

each keyboard shortcut should be unique in config

Current Behavior

it isn't

Possible Solution

fix ti

Steps to Reproduce (for bugs)

provide the same key in /usr/share conf. etc/xdg and $HOME/.config - maybe with different numbers

Context


^^^ i really don't like it 😎

Possible related to: #125

Imho the command/key combination should be unique, i have nothing against

ctrl a and ctrl b doing the same thing - might be legit, but i don't want to see

11 ctrl a action foo
12 crtl a action foo
13 crtl a action foo

System Information

latest git

@tsujan
Copy link
Member

tsujan commented Aug 22, 2019

I agree. The method used by LXQt didn't feel "natural" to me from start but it worked fine.

There are things we could learn from KDE -- at least, it could be inspiring -- but I'm afraid a backward compatible fix may not be possible in this case.

@agaida
Copy link
Member Author

agaida commented Aug 22, 2019

In this special case i really don't care about backward compat - and the to some extend we can be "backward-compatible" - read a current configuration and take the first valid entry for a key - drop all others. Done 😄

Edit: and my config is now a bit over 1000 lines ...

@tsujan
Copy link
Member

tsujan commented Aug 22, 2019

Yes, in addition to a new way of storing shortcuts, some code is needed to translate the old format to the new one.

@tsujan
Copy link
Member

tsujan commented Aug 22, 2019

Was it inherited from razor-Qt?

@agaida
Copy link
Member Author

agaida commented Aug 22, 2019

no clue, i guess so

@tsujan
Copy link
Member

tsujan commented Aug 22, 2019

OK, KDE's method isn't so inspirational ;)

[Data_5_1]
Comment=Comment
Enabled=true
Name=FeatherPad
Type=SIMPLE_ACTION_DATA

[Data_5_1Actions]
ActionsCount=1

[Data_5_1Actions0]
CommandURL=featherpad
Type=COMMAND_URL

[Data_5_1Conditions]
Comment=
ConditionsCount=0

[Data_5_1Triggers]
Comment=Simple_action
TriggersCount=1

[Data_5_1Triggers0]
Key=Meta+F
Type=SHORTCUT
Uuid={b8ad549f-39a6-49ee-a584-7ba3ec5130ae}

Int our method, shortcuts are section names. So far so good. It seems that .1, .2, .3,... are put there to support empty shortcuts but they defeat the purpose of choosing shortcuts as section names because they aren't unique anymore and can result in what's described in this report.

I think If, somehow, we remove .1, .2, .3,.. when shortcuts aren't empty, the problem should be solved because there couldn't be 2 sections with identical names.

@tsujan
Copy link
Member

tsujan commented Aug 22, 2019

Another method:

Use numbers as section names, put shortcuts inside sections, and add code to search for ambiguous shortcuts: either prevent them or show a warning. An example:

[56]
Comment=FeatherPad
Enabled=true
Exec=featherpad
Shortcut=Meta+F

@tsujan tsujan self-assigned this Aug 22, 2019
@tsujan
Copy link
Member

tsujan commented Aug 22, 2019

I assigned it to myself because I've dealt with shortcut ambiguity in other apps but it won't be for 0.15. If someone can make it efficient before 0.15, please do so!

@agaida
Copy link
Member Author

agaida commented Aug 22, 2019

Will result in a funny sorting ...

@tsujan
Copy link
Member

tsujan commented Aug 23, 2019

Sorting won't change in the GUI.

@tsujan
Copy link
Member

tsujan commented Aug 23, 2019

Please see if 794c780 fixes your problem and is worth making a PR.

As for .1, .2,..., they're needed by the current code because the code allows a key sequence to be repeated but decides which one should be activated.

@tsujan
Copy link
Member

tsujan commented Aug 23, 2019

In other words, the current code efficiently deals with ambiguity (although in a way that I don't like).

@agaida
Copy link
Member Author

agaida commented Aug 23, 2019

Meh - ok, i was a longterm database guy - and if anyone had introduced any kind of not needed sequenz number i had yelled at him long and loud - the point 'ambiguity' can be simply handled by the triplet {$key, $command/action, $activation-state} - not more, not less.

@tsujan
Copy link
Member

tsujan commented Aug 23, 2019

the point 'ambiguity' can be simply handled by the triplet {$key, $command/action, $activation-state} - not more, not less.

If there was only one way of doing each job, we would have only one OS and one program of each kind.

Loading identical shortcuts was a problem that can be fixed by the proposed patch but showing ambiguous ones while enabling only one of them is a feature — although I may not like it. It needs those numbers after dots.

Config files aren't for manual editing. Most of them can be edited but they're created by apps and can contain elements whose meanings aren't obvious to users. The presence of something like ♥ in a config file isn't a problem, let alone dots followed by numbers.

@agaida
Copy link
Member Author

agaida commented Aug 23, 2019

:)

@agaida
Copy link
Member Author

agaida commented Aug 23, 2019

Anyways, works like charm - and that's all what count - and i just discovered a new github thing that i like - never noticed it before:

@tsujan
Copy link
Member

tsujan commented Aug 23, 2019

Wasn't it there before? I don't remember.

@agaida
Copy link
Member Author

agaida commented Aug 23, 2019

no, never seen a branch overview which make it possible

  • to create a PR directly
  • delete branches
  • shows the differences in a short way
  • and shows meanwhile closed, but mentioned bugs

Really useful i think

@agaida agaida added this to Needs triage in Issues Aug 27, 2019
@agaida agaida moved this from Needs triage to High Prio in Issues Aug 27, 2019
@agaida
Copy link
Member Author

agaida commented Oct 14, 2019

done

@agaida agaida closed this as completed Oct 14, 2019
Issues automation moved this from High Prio to Closed Oct 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Issues
  
Closed
Development

No branches or pull requests

2 participants