search

HEnquist / camillagui

GNU General Public License v3.0
9 stars 1 forks source link

Implemented file manager #22

Closed JWahle closed 3 years ago

JWahle commented 3 years ago

Requires PR https://github.com/HEnquist/camillagui-backend/pull/17

If you wondered, why I did not create any PRs in the past few days, here is your explanation :) I implemented a file manager, that can:

Also added/updated some dependencies:

CamillaGUI-Filemanager

JWahle commented 3 years ago

Just to clarify: the load/save buttons do not affect the working config. Load just loads the config into the GUI - it still needs to be applied to take effect, and only then replaces the working_config. Save just saves the GUI state to the config file - you can change some settings, save to a config file and the working_config remains unaffected by this.

HEnquist commented 3 years ago

This looks nice! I will try it as soon as I can. One thing I noticed, the tooltips for the new buttons are very short. Could you add longer description to make it clear? I'm thinking of tooltip={'Upload files'} etc. Another thing, are "upload" and "download" good names to use? The old version seems to cause some confusion. We have quite a few "actions":

JWahle commented 3 years ago

I agree, that it is not quite clear, what really is happening. I'm not sure, if longer tooltips really solve this problem.

We have quite a few "actions":

I think that's the real problem here - together with the fact that we currently have many different places where config state can live:

There are actions to sync the GUI config with any other place (in both directions).

Another problem is, that it is currently not obvious, which config file was last loaded. This does not play well with moOde, that has its own mechanism of managing an active config among several config files.

Here is the current GUI in its full glory, with all the config sync actions: Sidepanel+Filemanager-actions

We could simplify it, so that the GUI keeps track of the currently active config and remove some of the buttons.

  1. Loading a config in the file manager (circle-arrow button) would load it to the GUI and make it active.
  2. Its filename is then shown in the "Config" box
  3. The "Config" box only contains buttons to
    • apply the GUI config to CDSP
    • save the GUI config to the currently active config file
    • both buttons could be disabled, when CDSP/active config are in sync with the GUI
    • we could even provide an optional "instant mode" like EqualizerAPO, where all the changes are applied and saved instantly. So no buttons would be needed in that case.

Sidepanel+Filemanager-actions-new

@HEnquist @bitkeeper what do you think?

bitkeeper commented 3 years ago

To help with the dicussion added a picture with all the 'players':

camilla_chain (1)

HEnquist commented 3 years ago

I have been thinking a bit about this, and here are some of my thoughts so far.

1) Loading a config in the file manager (circle-arrow button) would load it to the GUI and make it active.

Not sure about making it active, then you can't do stuff like easily taking a look at another config.

2) Its filename is then shown in the "Config" box

Good!

3) The "Config" box only contains buttons to

  • apply the GUI config to CDSP
  • save the GUI config to the currently active config file
  • both buttons could be disabled, when CDSP/active config are in sync with the GUI

Good, yes the "load from file" and "save to file" are redundant (and confusing!), those actions can be done equally well in the file manager. But I would keep the "load from CDSP button since there is no replacement for it (yet).

  • we could even provide an optional "instant mode" like EqualizerAPO, where all the changes are applied and saved instantly. So no buttons would be needed in that case.

Instant mode would be a nice feature. It needs the config to be valid to work of course. I would do this after improving the validation (both internal in the gui and the separate python-based validator I want to build). I have started on the python validator, using json schemas, and it looks quite promising.

JWahle commented 3 years ago
  1. Loading a config in the file manager (circle-arrow button) would load it to the GUI and make it active.

Not sure about making it active, then you can't do stuff like easily taking a look at another config.

Just to make sure, we are on the same page here - active only means, that it is now the default config, it still needs to be applied to CDSP afterwards. So you could load one config, have a look and then load the old one back. If it is possible to load a config, while having another config active, the save button would overwrite the active config. There is potential for accidental overwrites. Additionally, another button in the file panel is required to activate a config.

Instant mode would be a nice feature. It needs the config to be valid to work of course. I would do this after improving the validation.

What is the problem here? The backend only applies valid configs and the GUI indicates clearly when the config is not valid. I think this is pretty easy to implement with good user experience. It would of course be optional via a checkbox.

HEnquist commented 3 years ago

Just to make sure, we are on the same page here - active only means, that it is now the default config, it still needs to be applied to CDSP afterwards. So you could load one config, have a look and then load the old one back. If it is possible to load a config, while having another config active, the save button would overwrite the active config. There is potential for accidental overwrites. Additionally, another button in the file panel is required to activate a config.

Alright, yes I thought "activate" meant applying it to cdsp as well. No problem then.

What is the problem here? The backend only applies valid configs and the GUI indicates clearly when the config is not valid. I think this is pretty easy to implement with good user experience. It would of course be optional via a checkbox.

This was just about what order to do things, there is nothing preventing implementing immediate mode right away. I just think that the current feedback on config status isn't great, and there is a risk of confusion when for example changing the frequency of a filter, but nothing happens because you accidentally left another filter with a negative q-value.

JWahle commented 3 years ago

I implemented the handling of an active config and removed the load from/save to local config buttons from the sidepanel. A config becomes active, when it is loaded or saved. Its filename is then shown in the side panel and marked green in the file manager. The "Apply to CDSP" button also saves to the active config, if there is one. This is also described in the tooltip.

I think this should clear the confusion about what buttons do what.

Sidepanel+Filemanager2

bitkeeper commented 3 years ago

@JWahle this active_config direction is indeed a far better workflow then the previous one with working_config.

To improve the user feedback I think there are two cases left which could be improved ?: 1 Difference between cdsp and cdspgui setting Would it be possible to compare (online and offline) the active_config settings with the settings in CDSP itself ? In that the user would know that there is a mismatch ( and can presse load from or apply to cdsp to sync it)

2 The config in cdspgui has been edit but not save yet It would be nice if the user has an indicationg that the current config in the editor is changed, but not stored (to the current active config or another one)

JWahle commented 3 years ago

I agree with both - and have both on my TODO list. However, I'd like to do both in a different pull request.