PR: Restore widget shortcuts to Preferences and allow to change them on the fly (Shortcuts) #23024

ccordoba12 · 2024-11-19T03:17:08Z

Description of Changes

Those shortcuts were not shown in Spyder 6, as reported by several users.
That was a regression introduced by PR: Fix shortcuts for several Run and Debugger actions #22230, which unfortunately we overlooked.
This also fixes an important usability issue: changing those shortcuts didn't have an immediate effect (like all other shortcuts), but required an app restart. That issue was also present in Spyder 5 and it was very counterintuitive. Now those shortcuts will be updated on the fly.
Introduce a new ShortcutData dataclass to represent shortcut data internally and work more easily with it.
Change context of Switcher plugin shortcuts to be _, i.e. global (before their context was switcher but that made little sense).

Issue(s) Resolved

Affirmation

By submitting this Pull Request or typing my (user)name below,
I affirm the Developer Certificate of Origin
with respect to all commits and content included in this PR,
and understand I am releasing the same under Spyder's MIT (Expat) license.

I certify the above statement is true and correct: @ccordoba12

ccordoba12 · 2024-11-21T23:46:16Z

@dalthviz, this is ready for review. Please check that:

Shortcuts registered with register_shortcut_for_widget are shown in Preferences.
Changing those shortcuts works immediately (i.e. without a restart) in their respective widgets.

dalthviz · 2024-11-22T21:38:46Z

Gave this an initial check and seems like manually changing shortcuts is working and also shortcuts like duplicate line down are being displayed over the shortcuts preferences page 👍 However, when you modify a shortcut and then trigger a reset from the preferences page, the reset changes are not being honored inmediatly:

The current docstrings were not easy to understand.

- The previous solution, which used tuples to collect those data, was easy to break, because it required to put data in the right order; and undocumented, because it was unclear what kind of data had to be added in the tuple elements. - Those limitations made difficult to reason about shortcuts-related code.

- Before we only allowed class methods decorated with on_conf_change to do that. - But that's too limited if we need to use regular functions to observe an option. And that's precisely what this new method allows us to do.

This is necessary to add observers for specific shortcuts.

- That allows those shortcuts to be updated on the fly when they are changed in Preferences or directly with set_conf. - Also, fix inheritance of classes that inherit from SpyderShortcutsMixin to accomodate this change.

That was introducing an error in test_shortcut_for_widget_is_updated when run on CI due to the incorrectly named section.

This is to have feature parity of kwargs with the other methods of SpyderShortcutsMixin.

Also, add inline typing for several attributes of ConfigurationManager and remove associated comments.

ccordoba12 · 2024-11-25T18:05:10Z

However, when you modify a shortcut and then trigger a reset from the preferences page, the reset changes are not being honored inmediatly

Thanks for checking that @dalthviz! It should be fixed now.

I also took the opportunity to change the context of the Switcher shortcut actions to be _ because they are actually global shortcuts (the context was switcher).

dalthviz

Thanks @ccordoba12 ! Gave another local check to the reset shortcuts to default values and seems like things are working as expected now. Left a couple of questions and couple of suggestions like using an f-string at somepoint and an idea to extend a test but other than that this LGTM 👍

spyder/config/manager.py

spyder/plugins/editor/widgets/codeeditor/tests/test_codeeditor.py

spyder/config/manager.py

Also, remove some extra valid contexts that are not necessary anymore.

[ci skip]

- That prevents third-party plugin developers to set shortcuts that can't be processed by Spyder. - Print errors when instantiating CONF to allow third-party developers understand what's wrong with it. - Use an f-string when setting shortcuts for clarity.

pep8speaks · 2024-11-30T16:20:42Z

Hello @ccordoba12! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2024-11-30 16:21:36 UTC

ccordoba12 · 2024-11-30T17:44:13Z

@dalthviz, this is ready for another review. Besides addressing your comments, I also took the opportunity to fix other problems with shortcuts I found while doing that in my last four commits.

Specifically, I:

Fixed getting, saving and setting shortcut options with capitalized names (third-party developers could set them like that, so we need to be able to process them).
Add a test for registering widget shortcuts for external plugins (so that that doesn't break with future changes).
Validate that shortcut options have the right format (i.e. context/name) when setting defaults. This will ensure that third-party plugins use shortcuts in the way expected by Spyder.

dalthviz

Thanks @ccordoba12 ! Left a question regarding the usage of print instead of the login module in some error handling but other than that this seems like ready to be merged 👍

Will leave this approved so feel free to merge this is you think is ready.

dalthviz · 2024-12-01T18:20:11Z

spyder/config/manager.py


 try:
    CONF = ConfigurationManager()
 except Exception:
    from qtpy.QtWidgets import QApplication, QMessageBox

+    # Print traceback to show error in the terminal in case it's needed
+    print(traceback.format_exc())  # spyder: test-skip


Shouldn't we use the logging module for this?

I don't think so because this will help third-party developers to see, even without logging enabled, that their plugin config is incorrect, like this:

ccordoba12 · 2024-12-02T03:49:08Z

@meeseeksdev please backport to 6.x

…nces and allow to change them on the fly (Shortcuts)

…ferences and allow to change them on the fly (Shortcuts)) (#23137)

ccordoba12 added type:Bug component:Preferences labels Nov 19, 2024

ccordoba12 added this to the v6.0.3 milestone Nov 19, 2024

ccordoba12 self-assigned this Nov 19, 2024

ccordoba12 marked this pull request as draft November 19, 2024 03:17

ccordoba12 force-pushed the issue-22516 branch 7 times, most recently from 251ca02 to 8b3fe55 Compare November 21, 2024 17:15

ccordoba12 closed this Nov 21, 2024

ccordoba12 force-pushed the issue-22516 branch from 8b3fe55 to b18dc1a Compare November 21, 2024 22:37

ccordoba12 reopened this Nov 21, 2024

ccordoba12 marked this pull request as ready for review November 21, 2024 23:42

ccordoba12 requested a review from dalthviz November 21, 2024 23:42

dalthviz mentioned this pull request Nov 22, 2024

Help needed to make a plugin work for spyder 6.* #23013

Closed

ccordoba12 mentioned this pull request Nov 22, 2024

Change default keyboard shortcuts for macOS #10854

Open

ccordoba12 added 2 commits November 23, 2024 17:08

API: Improve docstrings of SpyderShortcutsMixin

bcbf1e2

The current docstrings were not easy to understand.

ccordoba12 force-pushed the issue-22516 branch from b18dc1a to 1f336d9 Compare November 24, 2024 17:09

ccordoba12 added 6 commits November 24, 2024 18:56

Shortcuts: Restore showing shortcuts for widgets in Preferences

dd9d7e8

Config: Notify when a shortcut is changed in our config system

c308cb9

This is necessary to add observers for specific shortcuts.

API: Add an observer for each shortcut registered for a widget

011bd0e

- That allows those shortcuts to be updated on the fly when they are changed in Preferences or directly with set_conf. - Also, fix inheritance of classes that inherit from SpyderShortcutsMixin to accomodate this change.

Testing: Check that shortcuts in CodeEditor are updated on the fly

9a54ef2

Testing: Remove setting CONF_SECTION for EditorStack/EditorSplitter

76a2d3b

That was introducing an error in test_shortcut_for_widget_is_updated when run on CI due to the incorrectly named section.

ccordoba12 added 3 commits November 24, 2024 19:00

Testing: Fix tests that use keyboard shortcuts

a34a26c

API: Add plugin_name kwarg to register_shortcut_for_widget

bba018e

This is to have feature parity of kwargs with the other methods of SpyderShortcutsMixin.

Config: Update widget shortcuts on the fly after resetting all shortcuts

b8d84e8

Also, add inline typing for several attributes of ConfigurationManager and remove associated comments.

ccordoba12 force-pushed the issue-22516 branch from 1f336d9 to b8d84e8 Compare November 25, 2024 02:17

ccordoba12 mentioned this pull request Nov 25, 2024

Custom shortcut Alt+Shift+Return doesn't work #23072

Closed

dalthviz reviewed Nov 25, 2024

View reviewed changes

spyder/config/manager.py Outdated Show resolved Hide resolved

spyder/plugins/editor/widgets/codeeditor/tests/test_codeeditor.py Show resolved Hide resolved

spyder/config/manager.py Show resolved Hide resolved

ccordoba12 added 3 commits November 28, 2024 16:20

Shortcuts: Fix saving shortcuts that use plugin_name from Preferences

b96896b

Switcher: Fix shortcut context of its actions to be global

77a5024

Also, remove some extra valid contexts that are not necessary anymore.

API: Fix several block comments so they're displayed in Spyder's Outline

a2b03a9

[ci skip]

ccordoba12 force-pushed the issue-22516 branch from 615f1ac to b0d9c04 Compare November 29, 2024 02:12

ccordoba12 added 3 commits November 29, 2024 11:54

Testing: Check that resetting shortcuts restore widget ones on the fly

819e5ab

Shortcuts: Fix getting/saving shortcuts with capitalized names

d68bfcf

Testing: Check registering widget shortcuts for external plugins

47ee224

ccordoba12 force-pushed the issue-22516 branch from b0d9c04 to 47ee224 Compare November 29, 2024 16:55

Testing: Check that an error is raised for invalid shortcut options

7b1a3fe

ccordoba12 force-pushed the issue-22516 branch from 43ef693 to 7b1a3fe Compare November 30, 2024 16:21

dalthviz approved these changes Dec 1, 2024

View reviewed changes

ccordoba12 merged commit c34f19c into spyder-ide:master Dec 2, 2024
17 checks passed

ccordoba12 deleted the issue-22516 branch December 2, 2024 03:47

meeseeksmachine pushed a commit to meeseeksmachine/spyder that referenced this pull request Dec 2, 2024

Backport PR spyder-ide#23024: PR: Restore widget shortcuts to Prefere…

c245c60

…nces and allow to change them on the fly (Shortcuts)

meeseeksmachine mentioned this pull request Dec 2, 2024

Backport PR #23024 on branch 6.x (PR: Restore widget shortcuts to Preferences and allow to change them on the fly (Shortcuts)) #23137

Merged

ccordoba12 pushed a commit that referenced this pull request Dec 2, 2024

Backport PR #23024 on branch 6.x (PR: Restore widget shortcuts to Pre…

3aedabf

…ferences and allow to change them on the fly (Shortcuts)) (#23137)

gepcel mentioned this pull request Dec 3, 2024

Shortcuts don't work for new files in the Editor #23151

Closed

This was referenced Dec 3, 2024

PR: Register shortcuts for widgets directly (API) #23161

Merged

PR: A couple of fixes for the Internal console #23217

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PR: Restore widget shortcuts to Preferences and allow to change them on the fly (Shortcuts) #23024

PR: Restore widget shortcuts to Preferences and allow to change them on the fly (Shortcuts) #23024

ccordoba12 commented Nov 19, 2024 •

edited

Loading

ccordoba12 commented Nov 21, 2024

dalthviz commented Nov 22, 2024

ccordoba12 commented Nov 25, 2024 •

edited

Loading

dalthviz left a comment

pep8speaks commented Nov 30, 2024 •

edited

Loading

ccordoba12 commented Nov 30, 2024 •

edited

Loading

dalthviz left a comment

dalthviz Dec 1, 2024

ccordoba12 Dec 2, 2024

ccordoba12 commented Dec 2, 2024

PR: Restore widget shortcuts to Preferences and allow to change them on the fly (Shortcuts) #23024

PR: Restore widget shortcuts to Preferences and allow to change them on the fly (Shortcuts) #23024

Conversation

ccordoba12 commented Nov 19, 2024 • edited Loading

Description of Changes

Issue(s) Resolved

Affirmation

ccordoba12 commented Nov 21, 2024

dalthviz commented Nov 22, 2024

ccordoba12 commented Nov 25, 2024 • edited Loading

dalthviz left a comment

Choose a reason for hiding this comment

pep8speaks commented Nov 30, 2024 • edited Loading

Comment last updated at 2024-11-30 16:21:36 UTC

ccordoba12 commented Nov 30, 2024 • edited Loading

dalthviz left a comment

Choose a reason for hiding this comment

dalthviz Dec 1, 2024

Choose a reason for hiding this comment

ccordoba12 Dec 2, 2024

Choose a reason for hiding this comment

ccordoba12 commented Dec 2, 2024

ccordoba12 commented Nov 19, 2024 •

edited

Loading

ccordoba12 commented Nov 25, 2024 •

edited

Loading

pep8speaks commented Nov 30, 2024 •

edited

Loading

ccordoba12 commented Nov 30, 2024 •

edited

Loading