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

refactor: Menus, Toolbars and Actions in MainWindow #1478

Merged
merged 17 commits into from
Jul 26, 2023
Merged

refactor: Menus, Toolbars and Actions in MainWindow #1478

merged 17 commits into from
Jul 26, 2023

Conversation

buhtz
Copy link
Member

@buhtz buhtz commented Jul 17, 2023

Partly reorganized code in MainWindow related to generation of menus, toolbars and the actions behind it. Also fixed some minor issues. The issues came up on the fly. I saw no way to separate them in a different Issue. But I keep to declare this PR/commit as "refactor" and not as "fix" because the Issues are tiny and didn't cause harm. See detailed list below.

I checked aryodas branch 1306_tray_icon_master_issue and can't find potential merge conflicts. But I'll wait for your feedback on that.

My first intention was to add a new menu to give the user the opportunity to modify the UI language. That is why the branch name is that way. But I realized I have to reorganize the code before doing this.

  • The code try to follow the Qt concept of separate "actions" (e.g. start snapshot) from visual UI elements (buttons, menus and their entries). An action is not equal to a slot or signal. Good example to see its advantage is how the menu and toolbar elements related to restoring a snapshot are enabled/disabled.
  • Generation of actions, menus and buttons are separated into multiple methods to reduce the amount of code in __init__().
  • Used underscores (_) for variable names (e.g. actions) because it is PEP8 convention. Camel case is suited for class names only.
  • Removed some instance attributes self.* because there is no need of them. Qt elements are managed by QMainWindow (or components of it) in the back. No need to keep references of them just to keep them alive.
  • Removed qttools.Menu class, which was a workaround to offer tooltips for menu entries. Qt 5.1 is able to do it itself.
  • The buttons "Take snapshot" (main toolbar) and "Restore" (file view toolbar) now have drop-down menus. It was not my idea. As I could see in the "old" code it was intended to be a dropdown button in the past. But it never (?) worked. I just fixed it and made it work. See my comparing screenshots below.
  • Main menu -> Snapshots: Removed the "Take Snapshots" submenu and integrated its only entry (Take snapshot with checksum) into the Snapshots menu.
  • Removed the Help button from main toolbar. All its content is present in the main menus "Help" menu. It was also drop-down tool button which dropdown menu doesn't work. I wasn't motivated to fix it. I just removed it. I can re-add it without problem if someone like.

This screencast do illustrate the drop-down toolbar buttons and the new "Snapshots" menu.

Peek 2023-07-17 14-09

Let's compare this with this screenshot of an older version of BIT. Take a look at the three red circles. There are "down arrows" on each of that buttons. But they didn't work in the current BIT version because the drop down menu setup code wasn't finished. It might be that it worked in an older version with Qt4 or something like that.

image

@buhtz buhtz self-assigned this Jul 17, 2023
@buhtz buhtz added Qt Qt bugs, code or features Bug Feedback needs user response, may be closed after timeout without a response Cosmetics appearance, icons, themes labels Jul 17, 2023
qt/app.py Show resolved Hide resolved
qt/app.py Show resolved Hide resolved
@buhtz buhtz added Discussion decision or consensus needed and removed Feedback needs user response, may be closed after timeout without a response labels Jul 17, 2023
@aryoda
Copy link
Contributor

aryoda commented Jul 17, 2023

@buhtz My sys-tray icon PR #1480 is merged now int dev. You could continue with this PR.

Very thoughtful UX improvement I think!

I am still trying to review the (huge) diff, please give me a few days (git diffs are not the best if much code has been moved around)

@buhtz
Copy link
Member Author

buhtz commented Jul 18, 2023

I am still trying to review the (huge) diff, please give me a few days (git diffs are not the best if much code has been moved around)

No problem and no hurry. Thanks.

Last time I used Qt was with C++ and Qt3 or 4 a 100 years ago. With this PR I learned some new things about Qt and how to handle it.

EDIT: Maybe viewing the diff in your terminal using git diff with --ignore-all-space and/or --ignore-blank-lines can help to improve the clearness of diff output.

@aryoda
Copy link
Contributor

aryoda commented Jul 25, 2023

@buhtz Incredible work to introduce such a clear QAction abstraction layer into the GUI!

It is very good like it is now and is ready to be merged.

I have just two questions to learn more about the reasons to implement the actions using an action_dict which creates action "on the fly" (meta programming).

  1. What is the reason and advantages to prefer this meta programming over using an action factory function basically containing the same code (+ using python code to create the actions then instead of using the action_dict) :

    backintime/qt/app.py

    Lines 542 to 562 in 8d418c5

    for attr in action_dict:
    ico, txt, slot, keys, tip = action_dict[attr]
    # Create action (with icon)
    action = QAction(ico, txt, self) if ico else \
    QAction(txt, self)
    # Connect handler function
    if slot:
    action.triggered.connect(slot)
    # Add keyboardshortcuts
    if keys:
    action.setShortcuts(keys)
    # Tooltip
    if tip:
    action.setToolTip(tip)
    # populate the action to "self"
    setattr(self, attr, action)

    # Let's assume the logic of above is implemented in the body of a factory function:
    # def action_factory(ico, txt, slot, keys, tip): ...
    
    self.act_take_snapshot = action_factory(icon.TAKE_SNAPSHOT, _('Take snapshot'),
                      self.btnTakeSnapshotClicked, ['Ctrl+S'], None))
    
  2. The action handler code could be implemented via a lamda/anonymous function instead of an explicit function since it most of the time is only short (passes arguments on the the back end function that knows nothing about the GUI and perhaps changes the GUI state). See e.g. this code:

    action_dict = {
                'act_take_snapshot': (
                    icon.TAKE_SNAPSHOT, _('Take snapshot'),
                    self.btnTakeSnapshotClicked, ['Ctrl+S'], None),
    ...
        def btnTakeSnapshotClicked(self):
            backintime.takeSnapshotAsync(self.config)
            self.updateTakeSnapshot(True)
    

qt/app.py Outdated Show resolved Hide resolved
qt/app.py Outdated Show resolved Hide resolved
@buhtz
Copy link
Member Author

buhtz commented Jul 26, 2023

I have just two questions to learn more about the reasons to implement the actions using an action_dict which creates action "on the fly" (meta programming).

  1. What is the reason and advantages to prefer this meta programming over using an action factory function basically containing the same code

Good question. I don't have a computer science like answer to this. It is just a habit. I saw something like this before I kind a liked it. I see no disadvantage of a factory function. Maybe it is because I see actions and gui elements as data. So I put this data elements in a data structure like a dict. It is more a feeling than a strong professional opinion. 😃

  1. The action handler code could be implemented via a lamda/anonymous function

Yes I thought the same. But I just stopped at this point to prevent the blowing up the PR.

@aryoda
Copy link
Contributor

aryoda commented Jul 26, 2023

  1. What is the reason and advantages to prefer this meta programming over using an action factory function basically containing the same code

Maybe it is because I see actions and gui elements as data. So I put this data elements in a data structure like a dict.

Absolutely OK so. One possible impact could be the missing or reduced intellisense and code navigation functionality in the IDE, let's try it and see how clever PyCharm is to handle this :-)

@buhtz buhtz merged commit 56ea86f into bit-team:dev Jul 26, 2023
1 check passed
@buhtz buhtz deleted the feat/langmenu branch July 26, 2023 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Cosmetics appearance, icons, themes Discussion decision or consensus needed Qt Qt bugs, code or features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants