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

Revert "BugFix: Repairs shortcuts to work 100% of the time" #5427

Merged
merged 1 commit into from Sep 12, 2021

Conversation

vadi2
Copy link
Member

@vadi2 vadi2 commented Sep 12, 2021

Reverts #3994.

It causes a regression:

think I found a bit of a regression after all. Shortcuts do not seem to be working like they used to in 4.12. I press Alt + R and get this

image

@vadi2 vadi2 requested review from a team as code owners September 12, 2021 05:29
@add-deployment-links
Copy link

add-deployment-links bot commented Sep 12, 2021

Hey there! Thanks for helping Mudlet improve. 🌟

Test versions

You can directly test the changes here:

No need to install anything - just unzip and run.
Let us know if it works well, and if it doesn't, please give details.

@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@vadi2 vadi2 merged commit 22576e1 into development Sep 12, 2021
@vadi2 vadi2 deleted the revert-3994-fixShortcutBreakingBug2 branch September 12, 2021 10:01
@vadi2
Copy link
Member Author

vadi2 commented Sep 12, 2021

Closes #5429.

@SlySven
Copy link
Member

SlySven commented Sep 18, 2021

TBH I am not convinced this is a fault of #3994 as that was just making OUR shortcuts work again - and then they are triggering this issue. I have seen it before, and I said so in Discord see https://www.discord.com/channels/283581582550237184/283582068334526464/853278452416118834 :

Has anyone else been getting this "Ambiguous short-cut" warning for the +L compact imput line toggle?

a follow-up message I posted shortly thereafter expanded on this https://www.discord.com/channels/283581582550237184/283582068334526464/853324221387767829 :

Aaarghh! I think it is the blasted auto-generated accelerator - I temporarily switched the +L one for the compact input line to + and found the original +L activated the main ToolBox menu - there isn't anything showing in Linux + KDE Plasma but I expect it is being treated as Too&lBox:

Notice how I say I temporarily redesignated one that was clashing (the compact input line one of <Alt>+L) and then found that it was being used for the "Toolbox" Mudlet main menu item - even though we have never set that up? As I said I think shortcuts are being generated automagically for us for things we do not want to have shortcuts for on some OSes...

@vadi2
Copy link
Member Author

vadi2 commented Sep 19, 2021

@Faenriis do shortcuts in PTB work for you again now?

@Xabre666
Copy link
Contributor

Yes, my develpmnent branch build and PTB are working again as they used to in 4.12. Sorry for not reporting it right away, thought that lack of my complaints would autmatically state that it works, I'm lazy. 😝

SlySven added a commit to SlySven/Mudlet that referenced this pull request Dec 3, 2021
…again

This should close Mudlet#3985, and *finally* Mudlet#649! I was prompted to look into
this as a result of what @demonnic reported in:
Mudlet#5715 (comment)

Previously the code was attempting to detect whether the main menu bar
itself was actually visible and using that to determine whether
EITHER: "key sequences" should be assigned to menu items and have the menu
items activate the desired slots (menu bar shown)
OR: "key sequences" should themselves be assigned to shortcuts that
directly call the desired slots (menu bar hidden)

This PR instead uses a state variable (boolean) and uses that to record
whether the menu bar is visible and only perform the changes required for
the above alternatives if the state is changed (or has not previously been
performed).

There have been several past attempts to get the short-cuts working:
* Mudlet#2071 monitored the state of the main toolbar and NOT the menu bar
* Mudlet#3994 changed to monitor the state of the menu bar but only enabled
(menu shown) or disabled (menu hidden) shortcuts that were assigned key
sequences and those shortcuts were connected directly to slots yet the key
sequences were assigned to menu items that themselves were ALSo connected
to the same slots.
* Mudlet#5427 reverted Mudlet#3994 so the code went back to that of Mudlet#2071

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
SlySven added a commit that referenced this pull request Dec 5, 2021
…again (#5723)

This should close #3985, and *finally* #649! I was prompted to look into
this as a result of what @demonnic reported in:
#5715 (comment)

Previously the code was attempting to detect whether the main menu bar
itself was actually visible and using that to determine whether
EITHER: "key sequences" should be assigned to menu items and have the menu
items activate the desired slots (menu bar shown)
OR: "key sequences" should themselves be assigned to shortcuts that
directly call the desired slots (menu bar hidden)

This PR instead uses a state variable (boolean) and uses that to record
whether the menu bar is visible and only perform the changes required for
the above alternatives if the state is changed (or has not previously been
performed).

There have been several past attempts to get the short-cuts working:
* #2071 monitored the state of the main toolbar and NOT the menu bar
* #3994 changed to monitor the state of the menu bar but only enabled
(menu shown) or disabled (menu hidden) shortcuts that were assigned key
sequences and those shortcuts were connected directly to slots yet the key
sequences were assigned to menu items that themselves were ALSo connected
to the same slots.
* #5427 reverted #3994 so the code went back to that of #2071

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
vadi2 pushed a commit to SlySven/Mudlet that referenced this pull request Jan 17, 2022
…again (Mudlet#5723)

This should close Mudlet#3985, and *finally* Mudlet#649! I was prompted to look into
this as a result of what @demonnic reported in:
Mudlet#5715 (comment)

Previously the code was attempting to detect whether the main menu bar
itself was actually visible and using that to determine whether
EITHER: "key sequences" should be assigned to menu items and have the menu
items activate the desired slots (menu bar shown)
OR: "key sequences" should themselves be assigned to shortcuts that
directly call the desired slots (menu bar hidden)

This PR instead uses a state variable (boolean) and uses that to record
whether the menu bar is visible and only perform the changes required for
the above alternatives if the state is changed (or has not previously been
performed).

There have been several past attempts to get the short-cuts working:
* Mudlet#2071 monitored the state of the main toolbar and NOT the menu bar
* Mudlet#3994 changed to monitor the state of the menu bar but only enabled
(menu shown) or disabled (menu hidden) shortcuts that were assigned key
sequences and those shortcuts were connected directly to slots yet the key
sequences were assigned to menu items that themselves were ALSo connected
to the same slots.
* Mudlet#5427 reverted Mudlet#3994 so the code went back to that of Mudlet#2071

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
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.

None yet

4 participants