Only show 'Close Library' if your in a library

[CarterPillow]

This is my first commit like ever on anything I know I made a lot of changes but I don't think this should have any effect outside of just only showing the close button when inside a library!

[Computerdores]

I would suggest also hiding the "Refresh Directories", "Save Library" and "Save Library Backup" Buttons

[CarterPillow]

I honestly did not even think of that but this hides all the buttons in the "files" menu that do not have any effect until you open a library I will look to see if there is anything else that might benefit from being hidden til you open a library.

[eivl]

Before opening a library

After

[CyanVoxel]

A better way to approach this in my opinion would be to simply disable the menu options instead of removing them entirely, for example how the Edit menu handles this:

This way the options don't change locations making them more difficult to find based on the state of the library, plus the user is made aware of all options whether they're presently available or not.

[CarterPillow]

Before Opening a Library

After

I looked at how the items were disabled in the version you showed (Alpha 9.4.0) and decided to add the "update_clipboard_actions" function over. Not the code from it since none of that is added yet just the function its self. This means that all the buttons will be enabled at first but since its the same function the ones that need to be disabled will be checked then disabled in the same code so its not a problem.

[CarterPillow]

I woke up to a lot of merges and this no longer working so ill have a look at what broke it and fix this pr lol

[CyanVoxel]

@CarterPillow Is this something you're still interested in working on?

[CarterPillow]

@CyanVoxel Actually yeah! I'll have a look and see if I can't update the code to work.

[CarterPillow]

That didn't take long! hopefully this is all working again lol.

[VasigaranAndAngel]

1. It's not enabling the actions when i close the library and reopen a library. to fix it call update_clipboard_actions() from the QtDriver.open_library() method so it will enable actions when opening a library.
2. format with ruff and do mypy check. (Install pre-commit if you haven’t yet.)

• why are there two options for enabling and disabling? what if there is only one option named toggle_action_state that stores a bool value? if it's True, the action should be enabled or disabled and if False, it should not.
• I prefer storing actions and/or menus (only if all the menu items are should be toggled) that need to be toggled when opening and closing the library, rather than enabling everything and explicitly disabling unwanted actions with the data() option.

btw, i personally like using QAction.property() rather than QAction.data() for its simplicity in this case.

[VasigaranAndAngel]

Suggested change

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

[seakrueger]

A very minor nit, but the function name calls the toolbar a clipboard, which usually refers to copy/paste. I feel update_menu_actions or update_toolbar_actions would be a more proper name. Thanks!

[CarterPillow]

1. It's not enabling the actions when i close the library and reopen a library. to fix it call update_clipboard_actions() from the QtDriver.open_library() method so it will enable actions when opening a library.
2. format with ruff and do mypy check. (Install pre-commit if you haven’t yet.)

• why are there two options for enabling and disabling? what if there is only one option named toggle_action_state that stores a bool value? if it's True, the action should be enabled or disabled and if False, it should not.
• I prefer storing actions and/or menus (only if all the menu items are should be toggled) that need to be toggled when opening and closing the library, rather than enabling everything and explicitly disabling unwanted actions with the data() option.

btw, i personally like using QAction.property() rather than QAction.data() for its simplicity in this case.

1. weird that issue was not happening for me but i added it there hopefully that will eliminate that bug for anyone else who might be getting it.

2. I will do that ruff thing now.

3. The reason I have 2 separate data options is because they have different behaviors. Autofill needs to be disabled on launch but not kept enabled when closing a library so if I just used 1 with true meaning it should be enabled/disabled then autofill would be kept open outside of libraries for example. I could just have 1 data variable for enable / disable and make it true if you want to keep it enabled after close and mark it false if you want to keep it disabled after open but i felt like that was overly complex and would just lead to a lot of confusion. This is just more explicit.
   I will look into property() vs data() though and make that change if it fits thank you for the recommendation. If i miss understood your point here about enable/disable let me know.

[CarterPillow]

A very minor nit, but the function name calls the toolbar a clipboard, which usually refers to copy/paste. I feel update_menu_actions or update_toolbar_actions would be a more proper name. Thanks!

This is a good idea. I think i only used update_clipboard_actions name because when i started this it was already a method but not sure. anyways now that the method is just this code I changed it to update_menu_actions.

[VasigaranAndAngel]

3. If i miss understood your point here about enable/disable let me know.

i meant only set the data if it’s a library related action. then enable or disable only those actions. alternatively you could just create a list of library related actions and toggle all of them together. it’d be simpler. no need to bother with any properties.

[CarterPillow]

i meant only set the data if it’s a library related action. then enable or disable only those actions. alternatively you could just create a list of library related actions and toggle all of them together. it’d be simpler. no need to bother with any properties.

I feel like 1 lists with properties would be more efficient then 2 lists.
and on the off chance that something would need to be both enabled and disabled idk if that is needed ever but this current code would account for this.

[CyanVoxel]

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.

[CyanVoxel]

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.

[CyanVoxel]

@CarterPillow Any updates or help needed on this?

[mashed5894]

working on this in #713