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

Only show 'Close Library' if your in a library #462

Closed
wants to merge 5 commits into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 56 additions & 2 deletions tagstudio/src/qt/ts_qt.py
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,8 @@ def __init__(self, backend, args):
self.filter = FilterState()
self.pages_count = 0

self.menus = []
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
self.menus = []
self.menus: list[QMenu] = []


self.scrollbar_pos = 0
self.thumb_size = 128
self.spacing = None
Expand Down Expand Up @@ -193,6 +195,7 @@ def open_library_from_dialog(self):
)
if dir not in (None, ""):
self.open_library(Path(dir))
self.update_menu_actions()

def signal_handler(self, sig, frame):
if sig in (SIGINT, SIGTERM, SIGQUIT):
Expand Down Expand Up @@ -275,6 +278,7 @@ def start(self) -> None:
# file_menu.addAction(QAction('&Open Library', menu_bar))

open_library_action = QAction("&Open/Create Library", menu_bar)
open_library_action.setProperty("disable", value=False)
open_library_action.triggered.connect(lambda: self.open_library_from_dialog())
open_library_action.setShortcut(
QtCore.QKeyCombination(
Expand Down Expand Up @@ -400,6 +404,7 @@ def create_dupe_files_modal():

# Macros Menu ==========================================================
self.autofill_action = QAction("Autofill", menu_bar)
self.autofill_action.setProperty("enable", value=False)
self.autofill_action.triggered.connect(
lambda: (
self.run_macros(MacroID.AUTOFILL, self.selected),
Expand Down Expand Up @@ -438,6 +443,13 @@ def create_folders_tags_modal():
help_menu.addAction(self.repo_action)
self.set_macro_menu_viability()

self.menus.append(file_menu)
self.menus.append(edit_menu)
self.menus.append(tools_menu)
self.menus.append(macros_menu)

self.update_menu_actions()

menu_bar.addMenu(file_menu)
menu_bar.addMenu(edit_menu)
menu_bar.addMenu(tools_menu)
Expand Down Expand Up @@ -474,7 +486,7 @@ def create_folders_tags_modal():
QColor("#9782ff"),
)
self.open_library(path_result.library_path)

self.update_menu_actions()
app.exec()
self.shutdown()

Expand All @@ -492,6 +504,47 @@ def show_error_message(self, message: str):
# Show the message box
msg_box.exec()

def update_menu_actions(self):
# To override the enabling and disabling of buttons
# there are 2 data options that can be added to a button
# but are not necessary

# name_of_action.setData({"enable":False})

# this one should be added to buttons that do not need to be enabled when a library
# is opened for example the autofil button only enables
# when clicked on a item not on library open

# name_of_action.setData({"disable":False})
# this one should be added to a button that needs to be enabled
# even when not in a library like the "open library button"

# if the button should be controlled here then just do not add any data objects

if self.lib.library_dir: # i think this should not return true when in a library?
# we are in a library! time to enable them.
for menu in self.menus:
for action in menu.actions():
# scary but all this dose is check if the aforementioned "enable" option is
# set to false and if it is then do not enable the button
action.setDisabled(
False
if action.property("enable") is None
else not action.property("enable")
if action.property("enable") is False
else False
)
Comment on lines +530 to +536
Copy link
Member

Choose a reason for hiding this comment

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

At this point I feel the amount of crisscrossing boolean logic is simply a trap waiting to confuse anyone in the future who will need to touch this code.

Let's take a step back and look at what we'd like to do:

  1. There's several menu options that we'd like to always enable whenever a library is open
  2. There's several menu options that we'd always like to disable whenever a library is not open
  3. There's a few menu options we'd like to only disable under specific library circumstances (i.e. there's nothing in the user's clipboard to "Paste" with

The use of setProperty/setData here is probably unnecessary to accomplish this. Instead we can simply add references to each menu item we know should always be disabled when a library is closed to a list. When a library is closed, we can call logic to iterate over that list and ensure that all options are disabled, regardless of their previous state. This list could be called something clear, such as "menus_to_disable_on_close". Likewise we could have another list that only contains references to menus that we know we always want open when a library is open. This would be iterated over in a similar fashion, this time enabling the options.

For any other cases, such as the "Paste" example mentioned earlier, we leave that logic up to the library. And if we were to have cases where an option should always be disabled (such as an incomplete feature, etc.) then we can just disable that on startup.

These lists would have clear names that let other people reading the code know what they're for. In the update method(s), the loops would be clearly iterating over them to perform the same unambiguous state change. If we ever find ourselves needing to account for a third predictable scenario, say if the program gains an additional "view mode" that changes what menu options are available, then we can easily follow the pattern we've laid out and add one or more additional lists of menu item references that we can cleanly iterate over during update methods. This ensures that developers can quickly see and understand the pattern that is here, and have an extensible pattern to work off of to build future features.

Copy link
Member

Choose a reason for hiding this comment

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

I also meant to mention: if the menu items that need to be disabled on close on enabled on open are the same, then we can store those in a single list that can have either disable or enable each option based on whether or not a library is open.

else:
# if we are not in a library then we need to disable all the unneeded menu buttons
for menu in self.menus:
for action in menu.actions():
# also scary but all this dose is check if the aforementioned "disable" option
# is set to false and if it is then do not disable this button
# when closing a library
action.setDisabled(
True if action.property("disable") is None else action.property("disable")
)

def init_library_window(self):
# self._init_landing_page() # Taken care of inside the widget now

Expand Down Expand Up @@ -586,7 +639,6 @@ def close_library(self, is_shutdown: bool = False):

self.lib.close()

self.thumb_job_queue.queue.clear()
if is_shutdown:
# no need to do other things on shutdown
return
Expand All @@ -606,6 +658,7 @@ def close_library(self, is_shutdown: bool = False):
self.main_window.statusbar.showMessage(
f"Library Closed ({format_timespan(end_time - start_time)})"
)
self.update_menu_actions()

def backup_library(self):
logger.info("Backing Up Library...")
Expand Down Expand Up @@ -1139,4 +1192,5 @@ def open_library(self, path: Path) -> LibraryStatus:
self.filter_items()

self.main_window.toggle_landing_page(enabled=False)
self.update_menu_actions()
return open_status