No backout for Default for All

ahall41 commented 9 months ago

Default for All (understandably) applies the changes immediately. If you click Cancel, then the defaults have already been set. In which case you should not need to click Apply.

It might be helpful to give a warning that this is going to happen, and also maybe a progress bar?

(Incidentally, I find it quite annoying to have to click on Apply then OK to get out of this dialog. Standard behaviour should be Apply makes the changes and leave the dialog open, OK makes the changes and closes it, Cancel closes without saving?)

ahall41 commented 9 months ago

Default also applies the changes without clicking Apply. In this case it is only 1 change, but should require Apply (as Cancel doesn't revert to the original setting).

oleg68 commented 9 months ago

Now all buttons Distribute audio groups, Default, Default for All, Reset and Apply change settings immediately to the running organ, so they can not be undone with Cancel.

So I'd replace the buttons Cancel and OK with the single button Close. @larspalo @rousseldenis do you agree?

ahall41 commented 9 months ago

You need a Cancel to ignore any changes you've made in the settings.

Two other connected issues I've noticed:

Select multiple items, enter a value into one of the boxes. Click Reset - nothing happens. This should perhaps clear the display? You have to either click Apply or Cancel to continue.
Select multiple items, enter a value into (say) Gain using +/-, then change your mind and clear it. Enter a value into (say) Amplitude. Now click Apply. Gain then gets applied as 0.00000 (which is perhaps undesirable). If you clear out the box, it should be ignored?

larspalo commented 9 months ago

@oleg68 I agree with replacing Cancel and OK with a single Close button.

However, I see some bad UI design in this dialog that we eventually need fixing. The buttons currently activate whenever an element in the tree is selected. They should activate/de-activate depending on context. For instance it's pointless to have Apply active when no change has occured. Also, I noted that the behaviour of the buttons are not really (explicitly) documented in the help section which should be another TODO.

oleg68 commented 9 months ago

@larspalo what is you opinion about adding a confirmation box to Default and Default to All?

ahall41 commented 9 months ago

Default is OK because it only affects one element (which you should then be able to Reset?). But Default All can undo everything you have just done, and needs to be applied with caution.

oleg68 commented 9 months ago

Seems, the name Default All is not clear. It affects the selected element and all it's subelements. Might Default with subs be better.

larspalo commented 9 months ago

@oleg68 I was under the impression that "Default to All" was intended to revert all the settings to the odf (or combined odf plus bundled .cmb), not only the selected element(s). That might indeed need a confirmation. But I've not checked that code recently so I cannot say for sure.

The selected element(s) "Default" without any confirmation is no problem, I think.

What exactly was the purpose of "Reset"? If that is what I thought "Default to All" to be, then it should be renamed to "Revert to Odf".

ahall41 commented 9 months ago

Reset puts the current values back as they were displayed (so if you change the wrong attribute or change your mind you can go back). It works OK for single elements, but (as identified) not for multiple. Default for all currently does apply to the selected items, being recursive down the tree. So if you click the top level, you can do the entire organ, but you could do just a rank - which I found quite useful yesterday making some adjustments. On a large organ (and a slowish PC) it takes a long time for the entire organ. I thought at first GO had started looping (so really needs a progress bar too)!

oleg68 commented 9 months ago

I was under the impression that "Default to All" was intended to revert all the settings to the odf (or combined odf plus bundled .cmb), not only the selected element(s)

No, not all. It reverts only the selected elements and all their subelements recursively. So the word 'All' is not precise.

I'd change it to Default with subs.

ahall41 commented 9 months ago

I'm not sure that Default with subs is all that helpful - what exactly do you mean by "subs"?

oleg68 commented 9 months ago

I'm not sure that Default with subs is all that helpful - what exactly do you mean by "subs"?

All subelements of selected elements, all subelements of the subelements and so on.

larspalo commented 9 months ago

I'd change it to Default with subs

"Default recursively" might be a better description. But I'm starting to wonder if we really need so many default options at all...

oleg68 commented 9 months ago

"Default recursively" might be a better description. But I'm starting to wonder if we really need so many default options at all...

What is your idea? To get rid of the 'Default' button?

larspalo commented 9 months ago

What is your idea? To get rid of the 'Default' button?

I think that a single button for all defaulting should really be enough if it's sensible enough to differ between what is currently selected in the tree (possibly with some kind of confirmation if multiple elements are selected or could be affected in unexpected ways). I'm not too concerned about the exact wording as long as it's behaviour is well documented. Also, the "Reset" button is somewhat ambiguous to me in this context and I personally don't find much use for it anyway as it's much too limited.

What we possibly end up with could be: "Distribute audio groups", "Default value(s)", "Apply" and then a "Close" like you suggested.

ahall41 commented 9 months ago

There is a subtle difference between setting the default for (say) a rank, and setting it for all the pipes in that rank. And a bug difference for setting the default for the organ and all its windchests, ranks and pipes. The current functionality is fine, except that it isn't clear that Default for All is applied immediately - a simple 'Are you sure' popup would suffice. Reset is useful if you've changed something and want to reset the display because you changed the wrong thing, wiothout having to Cancel and go back in to find the pipe you were changing.

larspalo commented 9 months ago

There is a subtle difference between setting the default for (say) a rank

Yes, but a single default button with a message dialog for elements that have children asking if values should be defaulted recursively could still be enough, I think. If no children exist then the change(s) could be applied immediately to the selected elements.

Though I personally don't find any use for the "Reset", and I've been working with GrandOrgue for some 14 years and never used it except for testing now, I won't mind if it is kept but both the "Reset" and "Apply" must get some logic to really detect if they should be activated or not - like: are there changes that can be Reset, are there any changes that can be applied? A sure sign of bad UI design is if there are buttons that can be clicked that does nothing...

oleg68 commented 8 months ago

I'm starting working on this issue now.

I'm going to do the following change:

[x] Rename the Reset button to Discard #1689
[x] Enable or disable the Discard and Apply buttons depending on existance of unapplied changes #1689
[x] Replace the Cancel and the OK buttons with a single Close button what will do the same as OK earlier. Pressing on Discard and then on Close will do the same as did Cancel before #1692
[x] Remove the Default for All button #1693
[X] Move the Default button left to the Distribute audio groups #1697
[x] when pressing on the Default button when the selected items have subitems, then a message box will appear with the text Some of selected objects have subobjects. Do you want to reset also subobjects to default values? with three buttons Cancel, Yes, No with dthe default No.
- Yes - reset the selected objects and all subobjects recursively
- No - reset only the the selected objects
- Cancel - don't change anything #1693
[X] Update the help image #1697

@ahall41 @larspalo @rousseldenis do you agree with this approach?

ahall41 commented 8 months ago

Can I get back in a few days? I need to think about it but am tied up at the moment.

larspalo commented 8 months ago

@ahall41 @larspalo @rousseldenis do you agree with this approach?

I think it looks very promising! The help file of course will need to be updated accordingly as some aspects weren't even covered before.

ahall41 commented 8 months ago

My only comment (and relates to the my initial "complaint") is to make it clear when you select Yes there is no going back (and also that it might take some time).

oleg68 commented 8 months ago

@ahall41 @larspalo @rousseldenis

What layout is better: the first or the second? buttons1

buttons2

ahall41 commented 8 months ago

The second. There is less wasted space at the bottom of the page (and it might all fit on my - by modern standards - low resolution monitor!). Perhaps a spacer between Distribute and Default? Common practice these days seems to have Apply, OK and Cancel. Apply makes the changes and keeps the dialogue open, OK makes the changes and closes it. And Cancel closes without making any changes. Perhaps this de facto standard could be applied?

oleg68 commented 8 months ago

Common practice these days seems to have Apply, OK and Cancel. Apply makes the changes and keeps the dialogue open, OK makes the changes and closes it. And Cancel closes without making any changes. Perhaps this de facto standard could be applied?

No, it cann't. Because Discard and Apply relates only to the selected element(s) and it is intended to make changes to many elements before closing this dialog. So the OK and Cancel buttons were replaced with Close.

larspalo commented 8 months ago

@oleg68 I think the second. But I'd like just a little distance between the buttons sideways even within the groups.

oleg68 commented 8 months ago

@larspalo @ahall41 Is this all right?

larspalo commented 8 months ago

@oleg68 Yes, I approve!

oleg68 commented 8 months ago

Thank you, @larspalo and @ahall41.

The new version of the dialog including the help changes will be available at https://github.com/oleg68/GrandOrgue-official/actions/runs/6677799862

But I cann't make a PR with them untill #1689, #1692, #1693 are merged. So approve them at first.

ahall41 commented 8 months ago

OK ... there's a small problem here. Select multiple items and change a value. Discard doesn't do anything, and you can't Close until you've pressed Apply (except you can use the close button (X)on the window). I also tried using Default to reset, but you still have to Apply before Close. Discard should clear the dialogue and disable Apply. BTW I'm not convinced that Discard is any less confusing than Reset ... might it be possible to use tool tips to make it clear exactly what we are trying to achieve ( Please add a warning that setting the Default for multiple objects may take (quite) some time. There's a typo in the Help - should read changes not hanges after Close. And the image for Default is somewhat mangled?

oleg68 commented 8 months ago

After #1695 I managed to make the dialog a little bit smaller:

organ_settings

oleg68 commented 8 months ago

Fixed with #1689, #1692, #1693, #1697 and #1701

ahall41 commented 8 months ago

Thanks @oleg68. There's just one thing I noticed - the image in the help for Default doesn't quite match the current dialogue?

oleg68 commented 8 months ago

@ahall41 Have you tried this intermediate build?

ahall41 commented 8 months ago

@oleg68 - yes. Its the bit under Default, which reads (in the Help)

Some selected objects have subobjects
Do you want the subobjects also to be reset to defaults?
Note resetting cann't be undone.

And in the dialogue

Some selected objects have subobjects
Do you want the subobjects also to be reset to to defaults?
Note: resetting cannot be undone.
Note: resetting all subobjects may take some time.

oleg68 commented 8 months ago

Thank you, @ahall41. I submitted #1711

GrandOrgue / grandorgue

No backout for Default for All #1674