[Screen] Apply button is always active (easy)

[diver]

Apply button is always active even if no changes have been made

[anevilyak]

I believe Apply is always enabled if set to "All Workspaces" rather than the current one. Is that the case for you?

[diver]

It is.

[modeenf]

Should this be like this?

[diver]

Still present in hrev39716.

[ctbeiser]

Still present in hrev43524.

[abhiin1947]

can you explain it a little further? I have made a patch, I just want to make sure that I am on the right track

[ScottyBeGSOC]

Patch to fix problem of apply button always enabled.

[mmadia]

ScottyBeGSOC would you be willing to recreate this patch using ​git format-patch. This will give you a better level of recognition for your contribution.  Thanks!

Can anyone comment on the substance of this patch and if it's OK to commit? The patch applies cleanly to current master (hrev45326). It appears to work as intended, enabling both Apply and Revert buttons only when changes are selected. Changing the values back to the original values will re-disable Apply and Revert.

[leavengood]

It seems weird that the same logic enables both buttons. But maybe that is correct, I'd have to test to see.

Code-wise the patch looks fine, except that if it really is the same logic for both buttons it should be calculated once and saved into a boolean, which can then be passed to the two SetEnabled calls.

[jscipione]

Replying to leavengood:

It seems weird that the same logic enables both buttons. But maybe that is correct, I'd have to test to see.

This patch is not correct, the Apply button should only check if the monitor fields are different from the default and not the workspace columns and rows. If you really want the Apply button to have a chance of being disabled when "All Workspaces" is selected you'll have to check the settings on each workspace to make sure that they match the current settings.

I checked BeOS R5 and the Apply button is always enabled when "All workspaces" is selected on that OS so IMHO this ticket is invalid, the current behavior is the correct behavior.

[bonefish]

Replying to jscipione:

I checked BeOS R5 and the Apply button is always enabled when "All workspaces" is selected on that OS so IMHO this ticket is invalid, the current behavior is the correct behavior.

Just because BeOS did it this way, doesn't mean it is correct. There's no point in copying bugs.

[Janus]

if I understand the problem, this is a possible solution

[pulkomandy]

Yes, sounds like the right way to go. Just a small change: count_workspaces() is not a cheap call (it asks the app_server for the workspace layout), so it should be called only once. Either make the loop go backwards, or (probably simpler) store the count in a variable and use that as the loop bound.

const int32 workspaceCount = count_workspaces();
for (int32 i = 0; i < workspaceCount; i++) {
...

[Janus]

I copied the loop from the _Apply function, do you think is a better solution to change both loops? In the _Apply is called only when the user press Apply (It is not a big penalty).

[Janus]

Fix both loops with your suggestion. Thanks.

[pulkomandy]

Applied in hrev48724. Thanks!

It would be nice if your patches included a more detailed commit message. I wrote one for this as you can see in the commit log. It is a good idea to explain what the patch does, what the problem was and how it is fixed.