Opened 10 years ago

Closed 5 years ago

#4231 closed bug (fixed)

[Screen] Apply button is always active (easy)

Reported by: diver Owned by: axeld
Priority: normal Milestone: R1
Component: Preferences/Screen Version: R1/Development
Keywords: GCI2011 Cc: ScottyBeGSOC
Blocked By: Blocking:
Has a Patch: yes Platform: All

Description

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

Attachments (3)

ScreenAppApplyButton.patch (782 bytes ) - added by ScottyBeGSOC 7 years ago.
Patch to fix problem of apply button always enabled.
0001-Fix-4231.patch (1.1 KB ) - added by Janus 5 years ago.
if I understand the problem, this is a possible solution
0001-Fix-4231.2.patch (1.5 KB ) - added by Janus 5 years ago.

Download all attachments as: .zip

Change History (21)

comment:1 by anevilyak, 10 years ago

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

comment:2 by diver, 10 years ago

It is.

comment:3 by modeenf, 10 years ago

Should this be like this?

comment:4 by diver, 10 years ago

Version: R1/pre-alpha1R1/Development

comment:5 by diver, 9 years ago

Still present in hrev39716.

comment:6 by ctbeiser, 8 years ago

Still present in hrev43524.

comment:7 by ctbeiser, 8 years ago

Keywords: GCI2011 added

comment:8 by diver, 7 years ago

Summary: [Screen] Apply button is always active[Screen] Apply button is always active (easy)

comment:9 by abhiin1947, 7 years ago

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

by ScottyBeGSOC, 7 years ago

Attachment: ScreenAppApplyButton.patch added

Patch to fix problem of apply button always enabled.

comment:10 by ScottyBeGSOC, 7 years ago

Has a Patch: set

comment:11 by mmadia, 7 years ago

Cc: ScottyBeGSOC added

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.

comment:12 by leavengood, 7 years ago

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.

in reply to:  12 ; comment:13 by jscipione, 6 years ago

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.

in reply to:  13 comment:14 by bonefish, 6 years ago

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.

by Janus, 5 years ago

Attachment: 0001-Fix-4231.patch added

if I understand the problem, this is a possible solution

comment:15 by pulkomandy, 5 years ago

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++) {
...

comment:16 by Janus, 5 years ago

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).

by Janus, 5 years ago

Attachment: 0001-Fix-4231.2.patch added

comment:17 by Janus, 5 years ago

Fix both loops with your suggestion. Thanks.

comment:18 by pulkomandy, 5 years ago

Resolution: fixed
Status: newclosed

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.

Note: See TracTickets for help on using tickets.