inconsistent behavior of Revert button in Backgrounds preflet (easy)

[wkornewald]

Open the Backgrounds preflet, change something, click Apply. Result: you can't click on Revert, anymore.

Revert should bring all workspaces back to the state when the application was started.

[korli]

• Revert lets you go back to the current workspace settings (even after having applied), and it's the good behavior.
• I don't understand the second point.

[wkornewald]

Replying to korli:

• Revert lets you go back to the current workspace settings (even after having applied), and it's the good behavior.

In order to analyze the problem, I've been playing with the Screen preflet which has the same behavior and I agree that for preflets which have an Apply button Revert should go back to the currently active configuration. What irritated me was that right after having clicked Apply I didn't get a chance to revert to the configuration before I pressed Apply. I don't see why this should be prohibited.

If you make new changes after having clicked Apply the Revert button should only undo the new changes (like it already does).

BTW, R5 has similar behavior for Screen (only Backgrounds is an exception): Revert always brings you back to the state at program start (even after having pressed Apply). I've tested this scheme, too, but I think that the suggestion above is more intuitive. In case you want to test that behavior, I've committed my modified version of Screen (it can still be reverted).

At least, in R5 Backgrounds is not consistent with the other preflets, so I think we should fix that behavior. What do you think?

• I don't understand the second point.

This is related to my suggestion above, but after having played around with the Screen preflet I think the second point is unnecessary and it's not worth the effort.

[wkornewald]

Revert should bring all workspaces back to the state when the application was started because that is more consistent with the behavior of all other preflets.

[max luebbe]

Replying to korli:

• Revert lets you go back to the current workspace settings (even after having applied), and it's the good behavior.
• I don't understand the second point.

I'm having trouble deducing what is the desired behavior from the comments here. This should be easy to fix with some clarification.

Example: Background is solid green when I open the app. I then change the sliders to blue and hit apply. I then change the sliders to gray.

At this point if I click Revert is the correct behavior:

a) Changing the background and sliders back to solid green b) changing the background to solid green keeping the sliders at current setting c) changing the sliders to blue, keeping the current background of blue d) something else

[wkornewald]

Replying to max luebbe:

I'm having trouble deducing what is the desired behavior from the comments here. This should be easy to fix with some clarification.

Example: Background is solid green when I open the app. I then change the sliders to blue and hit apply. I then change the sliders to gray.

At this point if I click Revert is the correct behavior:

a) Changing the background and sliders back to solid green b) changing the background to solid green keeping the sliders at current setting c) changing the sliders to blue, keeping the current background of blue d) something else

Let me clarify just in case you're still interested in fixing this: It should be (a) because all other preflets behave like that. Maybe you should ask on the mailing list before implementing it, though (I'm not part of the team, anymore, so my suggestion might not be accepted).

Well, an even better solution would be to have undo/redo where you can go through each change step-by-step, but that would have to be done for all preflets to not break consistency. Maybe with a reusable settings+history framework this could be implemented everywhere without a lot of effort.

[humdinger]

Short of an undo/redo history, I also think a) is the consistent behaviour. Plus, I'd do away with the Apply button and have all settings live.

[ekam]

Now all settings goes to initial state when revert button is pressed. There's still that apply button because I don't agree that all settings should go live immediately. Apply button is always enabled like in Screen preflet.

[stippi]

Can you explain more abut your patch? What at the fLast* members used for now? In many preflets, Revert will revert to those settings which have been last applied. But it makes sense to revert to the initial settings, too, regardless of how many times the user pressed Apply in the meantime. I am just wondering if the patch perhaps needs to clean up more remnants of the previous workings.

[ekam]

The main think (if you read earlier messages above) was to change revert to set all setting to initial state (state where they were when application was started and how it is with Screen preflet). "Revert will revert to those settings which have been last applied" That was the situation before fix and if it is how it should be then let it be like it was. Those fLast* members could be removed because at the moment there is no need to go back to previous applied state. I just made a _RevertTo function that takes all settings as an argument and because there was also discussions about undo/redo so I left also them there.

[ekam]

What I ment (in email) about improving inplementation was that there could be a wrapper for all those settings to pass to the revert function.

[ekam]

Someone should specify how reply should work in all preflets. If functionality in my patch is acceptable then I'll just remove old stuff there.

[stippi]

Sorry about the confusion... I am also fine with making Revert go back to the initial state. I would suggest to remove the fLast* members, though, since otherwise they might be confusing to someone trying to understand something about the code when something needs fixing or improving. If an Undo mechanism shall be designed, it will probably need more than just the fLast* members anyway. Thanks for working on this patch, it's very much appreciated!

[ekam]

Ok, thanks. I'll clean up the code and attach a new diff soon.

[ekam]

Sorry, I've been busy. I now checked out the code again and as I understand those fLast* variables are needed to set back the last menu item after user has pressed cancel button e.g when adding new image. Though, there was a bug in my code because I was using those variables in _RevertTo method.

[ekam]

Actually, I wasn't use any of those variables but I used one same name in function parameter so it was misleading

[stippi]

If you look at the patch here in Trac, you should see that it contains a lot of strange indentation. But it also contains other coding style violations. Please have another look at the guide lines (declaration of variables on separate lines, naming, no braces on single statement for-loops, ...). It would be great if you can fix those, thanks for working on this!

[ekam]

Sure, I will do that

[stippi]

Still something wrong with your editor settings, the patch changes all tabs to spaces where you touched code. This adds more hunks and it becomes harder to spot the actual changes. For the same reason, it is actually better to separate style cleanup and functional changes into separate patches. I don't want to put more work on you than necessary, though, so if you could just fix the spaces versus tabs issue, I'll have another look then, thanks!

[stippi]

Thanks for the new patch, now it was generally ok, though there were some places where you introduced whitespace at the end of lines or changed spacing unnecessarily. Most of the cleanup was ok, though. Thanks!

However, I have applied the patch here locally, and I find the new behavior worse than before, even though I agree with this ticket:

In the current Backgrounds preflet:

• Revert will be enabled as soon as you change anything in the current workspace.
• Apply will be enabled the same way.
• When you make some changes, and then switch to another workspace, Apply and Revert get disabled again, the changes are completely forgotten and when you switch back to the workspace with changes, the original settings are reflected in the GUI.
• When you click Apply, all is back to zero, the new settings are treated in just the same way as if the panel was closed and reopenened.

The last point is what this ticket is about. In any case, the behavior as is, is at least consistent.

With the patch applied, the behavior becomes a little weird:

• When you make changes without applying them and switch to another workspace, the button state is inconsistent, since you can apparently "Revert", even though no settings have changed for the new workspace.
• When you press Revert on the other workspace, nothing happens and when you switch back to the inital workspace with the changes, Revert is enabled, you can press it and now the settings are reverted to the initial state when the panel opened.
• Apply is initially not enabled when you open the panel, as expected.
• As soon as you make a change and revert it, Apply stays enabled.

So all in all, the patch improves something (solves this ticket), but overall the behavior of the enabling/disabling of the buttons is now more confusing.

IMHO, what should happen is that the code separates settings per workspace. For this I would introduce three sets of values: intial, changed and current. When the panel is opened, and when you switch to a workspace, one new set of values for this workspace is instantiated. The "initial" values are easy to assign, and it happens only once. The "current" set of values reflect the workspace as the user can see it on the screen. The "changed" values are adopted from the changes the user makes in UI. The Revert button is then enabled when either the current OR the changed values are unequal to the initial values. The Apply button is enabled whenever the current and changed values are not the same. There could be a third button "Undo" which is just enabled at the same time as Apply, and reverts the "changed" set to the "current" set. Just as Apply would adopt the settings for real and thereby assign the "changed" set to the "current" set. In other words, Undo would do what Revert does int he current panel.

[stippi]

Forgot to mention, if the Undo button is introduced, the Revert button would only be enabled if initial != current, in other words it stays disabled regardless of changes until you Apply.

[ekam]

Thanks for your comments. It was my mistake I forgot to tell you that diff I yesterday attached was just for verifying my changes in my editor. This one should be ok with indents and other code conventions. I also found out that I was made a bug while fixing those conventions.

Apply button functionality (always enabled) comes from Screen preflets functionality as someone mentioned it as reference (see above). Screen preflet also reverts always to initial state so this functionality comes also from there.

Those workspace related states I have to check but there was also this bug and it definitely could have messed up the functionality.

Are these new specifications (undo etc.) ones that I should follow now because this preflet will be then guite different than others.

[ekam]

You are right I have to fix that button behavior when switching between workspaces. Should I also add undo button and follow the new requirements you just specified? I suggest that we just fix that button behavior now and patch it as it then works like other preflets (like Screen preflet). You and (other gurus:)) should then make clear specs for all preflets and add new ticket for those enhancements.

[ekam]

If you agree I also suggest to store initial states of every workspace in a list so that it's possible to revert back to initial state in individual workspaces.

[stippi]

Yes, sounds like that would be good. Only revert in the workspace that the user sees.

[stippi]

Forgot to mention, Undo is just a suggestion, if you think it will be too different, then just ignore it. :-)

[ekam]

These things are never easy to solve as there's as many opinions as there is users. It would be nice to have guidelines also for these ones like how revert should act, what are the button names, where are buttons and other GUI components located etc. It looks like there's a need for usability expert ;)

[tangobravo]

Things are generally pretty consistent across the preflets. "Revert" will go back to the initial state, and changes are applied instantly. It's only those preflets which don't apply changes instantly that are the issue.

So: why can't we make backgrounds work the same way as the other preflets? There could be a small pause between updating the screen and changing parameters - say if you haven't altered any settings for a second then they get "applied". I can see why Apply is necessary in "screen" because changing resolutions is quite a slow process, but in Backgrounds I don't see why we can't do live updates.

[tangobravo]

A bit more thinking: Another reason the backgrounds case is confused is due to the single/all workspaces issue.

Perhaps another less confusing way of presenting the same options: Settings are live-applied to the current workspace. Another button, "Apply to All Workspaces", is used for the more destructive step of applying to all.

Maybe two revert buttons - "Revert This Workspace", "Revert All Workspaces" to handle the Revert confusion. Alternatively a single Revert button, but if both "all" and "this" have been changed (to different things) then pop up a question box.

[ekam]

Please, could you guys make a final decision how this preflet should work so that someone could implement it?

[stippi]

So you mean to remove the Workspace BMenuField? That could work. But all in all I think the preflet is fine as it is, precisely because settings take affect with a quite noticable delay. Hence the Apply button, it will only be worse with less capable computers. All in all, I still think what I outlined above would work.

[tangobravo]

Replying to stippi:

So you mean to remove the Workspace BMenuField? That could work.

Yes, that's what I meant.

Your description sounds good too (though I'd lose the "Undo" bit, and just have revert always reset to initial state).

There is still an issue of whether Revert should only affect the single workspace or all workspaces - I guess that could just be decided based on the menu item selected.

btw ekam, I'm not really a Haiku developer, I was just throwing ideas out there. So what I say should never be treated as a final specification for the desired behaviour :)

[ekam]

So, if workspace menu will be removed could there be checkbox 'All Workspaces'? Apply and Reply affects all workspaces when that is checked.

[ekam]

tangobravo: That's the problem when there's lots of ideas but nobody to decide what to do.

[tangobravo]

Replying to ekam:

tangobravo: That's the problem when there's lots of ideas but nobody to decide what to do.

There will be a decision, stippi is likely to be the one to make the call in the end. I don't see a problem with suggesting alternatives though.

It seems you both prefer not to have settings applied live, which means keeping the apply button, and therefore there's no need to change the menu field.

As the guy doing the implementation, you'd probably be the one to decide the exact scheme used. Following Stephan's suggestion outlined above seems a good plan to me: "IMHO, what should happen is that the code separates settings per workspace. For this I would introduce three sets of values: intial, changed and current. When the panel is opened, and when you switch to a workspace, one new set of values for this workspace is instantiated. The "initial" values are easy to assign, and it happens only once. The "current" set of values reflect the workspace as the user can see it on the screen. The "changed" values are adopted from the changes the user makes in UI. The Revert button is then enabled when either the current OR the changed values are unequal to the initial values. The Apply button is enabled whenever the current and changed values are not the same."

All that needs further specifying is the revert behaviour. I would suggest: Revert will set the current workspace back to it's initial state, or all workspaces if "All Workspaces" is selected in the menu field.

I've left out Stippi's Undo suggestion - I don't think it would be useful often, and will add confusion for the user as to the difference between undo and revert. Often with these things it's hard to know how it will feel until it's actually been implemented though.

[ekam]

There will be a little break in implementation now but I hope I can attach a final diff next week.

Every workspace specific initial states are now stored and also not yet applied panel settings will be stored so that user can continue editing settings when switches back to previous workspace.

[ekam]

The break was little bit longer than I thought but now I have time to continue the implementation again.

[ekam]

It has been challenging to find time for this but for your information, I'm still working with this ticket.

[ekam]

Tangobravo's suggestion was to revert all workspaces when "All Workspaces" is selected in the menu field but I think it's not vice to revert something when you can't see the result so I didn't do it. I have found out that I really don't have enough time to work with Haiku which is a pity. I hope you can use this patch.

[ekam]

I'm sorry, but I have wait and wait that someone could check my patch but now after 8 weeks I'm so disappointed that I decided to remove that patch. I know that everyone is busy with more important issues but if you want new people to join you should have at least a little interest of what they are doing. Despite of that I sincerely wish all the best for your future endeavors.

[tqh]

Ah that's to bad.
Believe me there is interest, but its hard to juggle all the things that are going on. I think many of the devs have very much to do at the moment (both in real life and with Haiku) and in the summer many people are away.
I know how it is waiting for others when you are working on code, but I can't really say that you can expect much faster handling at the moment. Many of us are reading the bug update emails, so asking in a comment once in a while might get you a response faster.

Good luck to you too.

[stippi]

Markku has sent me many emails in private, asking me to review his patch, and I have told him that I am trying to make the time. It's just not good when single individuals are becoming a road-block to other people, but I have not asked to be this person. I knew it was very important for someone to review this patch in order not to loose a potential new contributor and I had planned to suggest to Markku to ask on the development list instead of contacting me in private all the time, because I hate being a road-block. I am a father now and my priorities are just different. Sorry that I have disappointed you, Markku, but I cannot do anything about it.

[ekam]

Thanks tgh and Stephan for your answers.

Stephan has been the only person who have tried to help me and he has also asked others to be more active to review patches. It was a big step for me to start coding Haiku as I didn't have knowledge of BeOS and I haven't done any serious job by c++ before this so all your support was crucial so I thank you.

I have lots off other things to do as I have 8 years old son who plays football (which takes a lot of my time), I'm playing in a band, trying to finish my studies at the University of Applied Sciences and of course I'm working every day.

I'm very sorry about my behavior yesterday and I promise to add that patch back (I don't have it here now).

BTW, there has been couple of commits by others after I attached that patch so do you want me to merge those changes to mine first? Although, we might be in a same situation if there will be another 8 weeks before someone has time to check the patch. I also don't have much time for this now.

[ekam]

It's back there. Hope someone could check it someday.

[ctbeiser]

I can verify that this is still performing as the initial bug description describes it in hrev43524.

[ekam64]

ctbeiser, specifications changed often during the fix so please read all the comments.

[pulkomandy]

Reassigning to Janus as he is probably interested in these UI things.

[pulkomandy]

Patch migrated to Gerrit: ​https://review.haiku-os.org/c/haiku/+/2604