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

[WIP] PR: Changes for handling Shortcuts with traitlets in consoleWidget #621

Open
wants to merge 30 commits into
base: main
Choose a base branch
from

Conversation

jsbautista
Copy link
Contributor

No description provided.

@jsbautista jsbautista changed the title [PR] : Changes for handling Shortcuts with traitlets in consoleWidget [WIP] PR: Changes for handling Shortcuts with traitlets in consoleWidget Sep 2, 2024
Copy link
Collaborator

@dalthviz dalthviz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank @jsbautista for your work here! Added some suggestions that should prevent the changes here to cause a segfault. If you have any question about the suggested changes let me know!

qtconsole/console_widget.py Outdated Show resolved Hide resolved
qtconsole/console_widget.py Outdated Show resolved Hide resolved
qtconsole/console_widget.py Outdated Show resolved Hide resolved
qtconsole/console_widget.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@dalthviz dalthviz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the latest changes @jsbautista ! Left some new comments/suggestions related with the method that gathers the different actions with shortcuts over a dictionary. Also, it could be nice to add a GIF or a command/flag/config file over the PR description to show an example of how changing the shortcuts could be done using the work being done here.

@@ -5,7 +5,7 @@

from qtpy import QtCore, QtGui

from traitlets import HasTraits, TraitType
from traitlets import default, HasTraits, TraitType, Unicode, observe
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could it be that these added imports are not needed anymore?

Comment on lines 241 to 267
self.window_shortcuts['shortcut_new_kernel_tab'] = self.shortcut_new_kernel_tab
self.window_shortcuts['shortcut_slave_kernel_tab'] = self.shortcut_slave_kernel_tab
self.window_shortcuts['shortcut_existing_kernel_tab'] = self.shortcut_existing_kernel_tab
self.window_shortcuts['shortcut_close'] = self.shortcut_close
self.window_shortcuts['shortcut_save'] = self.shortcut_save
self.window_shortcuts['shortcut_print'] = self.shortcut_print
self.window_shortcuts['shortcut_quit'] = self.shortcut_quit
self.window_shortcuts['shortcut_undo'] = self.shortcut_undo
self.window_shortcuts['shortcut_redo'] = self.shortcut_redo
self.window_shortcuts['shortcut_cut'] = self.shortcut_cut
self.window_shortcuts['shortcut_copy'] = self.shortcut_copy
self.window_shortcuts['shortcut_copy_raw'] = self.shortcut_copy_raw
self.window_shortcuts['shortcut_paste'] = self.shortcut_paste
self.window_shortcuts['shortcut_select_all'] = self.shortcut_select_all
self.window_shortcuts['shortcut_ctrl_shift_m'] = self.shortcut_ctrl_shift_m
self.window_shortcuts['shortcut_full_screen'] = self.shortcut_full_screen
self.window_shortcuts['shortcut_zoom_in'] = self.shortcut_zoom_in
self.window_shortcuts['shortcut_zoom_out'] = self.shortcut_zoom_out
self.window_shortcuts['shortcut_reset_font_size'] = self.shortcut_reset_font_size
self.window_shortcuts['shortcut_clear'] = self.shortcut_clear
self.window_shortcuts['shortcut_interrupt_kernel'] = self.shortcut_interrupt_kernel
self.window_shortcuts['shortcut_restart_kernel'] = self.shortcut_restart_kernel
self.window_shortcuts['shortcut_minimize'] = self.shortcut_minimize
self.window_shortcuts['shortcut_prev_tab'] = self.shortcut_prev_tab
self.window_shortcuts['shortcut_next_tab'] = self.shortcut_next_tab
self.window_shortcuts['shortcut_rename_window'] = self.shortcut_rename_window
self.window_shortcuts['shortcut_rename_current_tab'] = self.shortcut_rename_current_tab
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also make this method return a dict instead of initializing a class attribute. So maybe calling the method something like _get_shortcuts and doing something like:

        shortcuts = {
            'shortcut_new_kernel_tab': self.shortcut_new_kernel_tab,
            'shortcut_slave_kernel_tab': self.shortcut_slave_kernel_tab,
            ...
        }
        return shortcuts

Comment on lines 383 to 384
self.initialize_window_shortcuts()
self.window = MainWindow(self.app, shortcuts=self.window_shortcuts,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe instead of defining an attribute we could make the initialize_window_shortcuts to just be a utility method that returns a dict with the shortcuts and we call it here. So something like:

Suggested change
self.initialize_window_shortcuts()
self.window = MainWindow(self.app, shortcuts=self.window_shortcuts,
self.window = MainWindow(self.app, shortcuts=self._get_shortcuts(),

#--------------------------------------------------------------------------
# Shortcuts definition
#--------------------------------------------------------------------------
window_shortcuts={}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the idea of making the method return the dict with the shortcuts this would not be needed anymore

# Shortcuts definition
#--------------------------------------------------------------------------
window_shortcuts={}
shortcut_new_kernel_tab = Unicode('Ctrl+T').tag(config=True)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking how other traitlets variables are being defined around, seems like we could also write this like:

... = Unicode('Ctrl+T', config=True)

We could even write some help strings for these new variables. So something like:

... = Unicode('Ctrl+T', config=True, help="Shortcut to create a new kernel tab")

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.

2 participants