Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#3511 closed enhancement (fixed)

[PATCH]Give the possibility not to save options for one session.

Reported by: Stan Owned by: mimo
Priority: Should Have Milestone: Alpha 20
Component: UI & Simulation Keywords: patch
Cc: Patch:

Description

As reported by mimo on IRC, currently everything single option is saved for next session.

It would be nice to ignore change for one session with a tickable option in the settings window such as

Ignore changes (Though this might be a little misleading)

This is different from the option of not saving session changes cause here it will only happen once.

Notice this setting should be always false.

Attachments (5)

patch_3511.patch (1.9 KB ) - added by Pilzschaf 8 years ago.
ticket-3511-full.patch (12.8 KB ) - added by mimo 8 years ago.
ticket-3511-noswitch.patch (9.6 KB ) - added by mimo 8 years ago.
cleanOptions.diff (8.3 KB ) - added by mimo 8 years ago.
cleanOptions.2.diff (9.0 KB ) - added by mimo 8 years ago.
same as before, but with detection of changes to enable reload and save button only when needed

Download all attachments as: .zip

Change History (26)

comment:1 by elexis, 8 years ago

See also #3324

comment:2 by leper, 8 years ago

Keywords: simple added

Having another button that does not reset the settings like Cancel, but does not save would be the way to go.

comment:3 by wraitii, 8 years ago

Milestone: BacklogAlpha 20

by Pilzschaf, 8 years ago

Attachment: patch_3511.patch added

comment:4 by Pilzschaf, 8 years ago

Keywords: review patch added; simple removed
Milestone: Alpha 20Alpha 19
Summary: Give the possibility not to save options for one session.[PATCH]Give the possibility not to save options for one session.

comment:5 by elexis, 8 years ago

Milestone: Alpha 19Alpha 20

Thanks for the patch. Since a19 is already being packaged and this isn't a severe error, we can't include it in a19 unfortunately.

comment:6 by mimo, 8 years ago

As this patch give us the possibility to change the settings without saving them, it would be useful to also add a button to reload the saved options.

comment:7 by leper, 8 years ago

Keywords: review removed

To save for this session you do not need to write the config file at all. (This assumes no other code writes to the config file though (if some does that should likely be fixed))

Also see mimo's comment above.

comment:8 by mimo, 8 years ago

Any update on this patch ? would be nice to have it for a20.

comment:9 by mimo, 8 years ago

I've uploaded a different patch which also fixes #2451 and also some cases where the graphic settings were not saved in the config file. ticket-3511-full.patch is the full patch, while ticket-3511-noswitch.patch is there to ease the review (same patch without the fix for switch indentation).

by mimo, 8 years ago

Attachment: ticket-3511-full.patch added

by mimo, 8 years ago

Attachment: ticket-3511-noswitch.patch added

comment:10 by mimo, 8 years ago

Owner: set to mimo
Resolution: fixed
Status: newclosed

In 17625:

allows changing options for one session without having to save them, fixes #3511
changes on text or number options are now detected without any need of typing Return, fixes #2451
graphic options (using the renderer type) are now properly saved when changed
sound options (using the function type) are still not saved

comment:11 by Josh, 8 years ago

Reload is a bit misleading, as you don't say what your reloading. Are you reloading the default values? Are you reloading initial settings? I think people would be more used to a Cancel button.

I'm also rather unclear on what the changes to the renderer configuration type in r17625 do. As I read it, you added unreachable/duplicate code because the renderer already automatically saves its config.

comment:12 by mimo, 8 years ago

Reloading the last saved user values (ok, we can make the tooltip more clear). After playing (and possibly screwing up) your options, i think reverting to your saved (user) options is what you want.

I don't think the renderer saves its values (at least it was not saved by the previous save button, so how and where would it be saved ?). Now, these values are saved in the user config file, so they can be restored.

Last edited 8 years ago by mimo (previous) (diff)

in reply to:  12 ; comment:13 by Josh, 8 years ago

Replying to mimo:

.... I don't think the renderer saves its values (at least it was not saved by the previous save button, so how and where would it be saved ?). Now, these values are saved in the user config file, so they can be restored.

The renderer options save fine for me right now without updating to your revision.

comment:14 by mimo, 8 years ago

I've just tested to revert, and indeed renderer options were correctly saved. Don't know what happened, maybe a corrupted user.cfg in my tests. I will double check this tomorrow, and revert some renderer changes if not needed.

Last edited 8 years ago by mimo (previous) (diff)

comment:15 by fabio, 8 years ago

Thanks for working on this, the options menu really needed some update. I add here some other problems with options menu (possibly worth new tickets but reporting here currently since it still WIP) I notice.

  • I would rename "Reload default values from file" to "Reload previous saved settings" and "Save changes to file" to "Save changes" or "Save settings" (avoiding the file term);
  • initial settings don't match the effective renderer option. To reproduce:
    • remove user.cfg (so that default value will be used)
    • if under Linux with mesa drivers start the game with LIBGL_ALWAYS_SOFTWARE=1 ./0ad this should disable most graphics features (alternatively tweak hwdetecs.js to disable some settings for your card);
    • start a match and notice that, for example, shadows are really disabled;
    • open options menu and notice however that shadows are enabled; -> the options menu is not correctly loaded on default values for many other options.
  • it would be nice to have a button to "Restore default game settings. REQUIRES GAME RESTART" that will remove any settings in user.cfg (excluding the lobby login and password to avoid users losing them), this way users that tampered their settings have a quick way to restore the default;
  • sound settings value should probably be remapped to a 0-10 (possibly 11) scale both in options menu and config files; the engine should then remap this values to its internal values (it may be that current values already have meaning in the scale 0-10 or are percentage with a maximum of 1?);
  • sound settings should use a slider or a combo box rather than textual field (it is not clear what is the range currently);
  • missing useful material quality setting in option menu (already filed as #2596), possibly using a slider/combo box.

in reply to:  13 comment:16 by mimo, 8 years ago

Replying to Josh:

Replying to mimo:

.... I don't think the renderer saves its values (at least it was not saved by the previous save button, so how and where would it be saved ?). Now, these values are saved in the user config file, so they can be restored.

The renderer options save fine for me right now without updating to your revision.

In fact, I now remember why i changed it. It was not the Save, but the Cancel changes which was completely broken in the old version. Try to change some renderer options and then cancel them. Nothing will be canceled as can be checked by reopening the option menu. And that is expected because the Cancel only reload the options from the user file. That is really a (bad) consequence of having two different saved options.

With the changes, when canceling (or reloading as it is called now), i give now priority to the config options (and not the renderer ones as in the old version). But maybe it would be more consistent to always give priority to the config options and not the renderer ones.

by mimo, 8 years ago

Attachment: cleanOptions.diff added

comment:17 by mimo, 8 years ago

cleanOptions.diff is a patch which revisits the logics of options.js, simplifying it and removing the need of the merge functions.

by mimo, 8 years ago

Attachment: cleanOptions.2.diff added

same as before, but with detection of changes to enable reload and save button only when needed

comment:18 by mimo, 8 years ago

fabio, i've tried hacking hwdetect to start with most options disabled, and the display of options works for me. Can you check again with the latest patch (either cleanOptions.diff or cleanOptions.2.diff, the second one needs a compile), which i've used for my tests and which includes a fix for that problem.

For the four last bullets of your comment, could you open a new ticket.

comment:19 by fabio, 8 years ago

mimo: great, I can confirm that your patch indeed fixes the value of initial settings in options menu! I tried cleanOptions.diff before and after applying it and with settings that should either initially enabled or disabled and all are OK now.

comment:20 by mimo, 8 years ago

In 17645:

Rework the logics of options loading to allow more flexibility (i.e. adding a min or max value for number inputs)
Fix some problems when reloading saved options, refs #3511
Add the material quality option, refs #3737
Reload and Save buttons are only enabled when some options have changed.

comment:21 by elexis, 7 years ago

In 20091:

Allow the GUI to subscribe to text edit events to redeem three workarounds in the options page.

Differential Revision: https://code.wildfiregames.com/D860
Refs #2451, #3511, r17645 (registerChanges), rP17625 (onMouseLeave), rP10108 (onPress)
Reviewed By: Vladislav

Note: See TracTickets for help on using tickets.