Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OpenMW-CS: Add option to reset settings to default (Feature #3275, #3682) #1286

Merged
merged 18 commits into from May 12, 2017

Conversation

PlutonicOverkill
Copy link
Contributor

https://bugs.openmw.org/issues/3275
https://bugs.openmw.org/issues/3682
Adds a button at the top of each settings category to reset all settings in that category to the default. It mostly works, except that when the reset button is clicked in the "Key Bindings" category, the displayed settings do not change and go out of sync with the actual settings. This seems to be because the settings in ShortcutManager are not updated when the settings in Settings::Manager::mUserSettings are. How could I go about fixing this?

Would it be necessary to add a confirmation dialog in case the user accidentally clicks the reset button? Also, should the reset button for the "Key Bindings" section reset all of the key bindings or just the key bindings in the current subcategory?

@zinnschlag
Copy link
Contributor

Problematic. First I don't think re-building the UI is the correct way to handle it (not to mention that your implementation isn't exception-safe and could potentially end up with a dangling pointer). This is way too disruptive.

The correct way of handling this feature would be to enhance the Setting class with a new function that that sets the widget to the state of the setting. This would then have to be implemented in all subclasses of Setting and the subclasses would probably have to store a pointer to the last created widget.
Not sure what is going on with the shortcuts. Maybe the problem goes away with the implementation proposed above? If not we can have another look at it.

As for the UI, no confirmation dialogue is required. But I don't think a button is a good idea either. How about a context menu (both for the page and for the item in the list on the left side)? That would also resolve the issue about the scope of resetting keybindings. You could have two menu items for the page context menu: Reset all keybdings, Reset keybindings for category x (with x being the currently selected category).

@PlutonicOverkill
Copy link
Contributor Author

OK, done. A few things: to get the context menu for the settings pages to appear you have to click on the actual settings, it won't work if you just click in a blank area. For the key bindings page, I made the "reset category" option reset all of the key bindings to keep it consistent with the context menu on the list item and to minimize unnecessary complexity. Also, I'm wondering why mList is a member of CSVPrefs::Dialogue when it is only used in one function. Is it because of some Qt cleanup code in the destructor? If it is necessary for it to be a member, I can change it back.

@zinnschlag
Copy link
Contributor

to get the context menu for the settings pages to appear you have to click on the actual settings, it won't work if you just click in a blank area

That is an issue. Impacts the discoverability of this feature quite badly. Can you look into that a bit further? I am sure there must be a way to work around this limitation.

Regarding mList: Probably a left over from an earlier version. With the code base as it is right now, mList is redundant.

@PlutonicOverkill
Copy link
Contributor Author

After a lot of trial and error, I found the easiest way of doing this is just creating a top level widget that resizes to fit the entire page, and then creating the actual settings page as a child widget with a fixed size. I did notice a couple of other annoyances though. One is that clicking a button in the Key Bindings section and then right-clicking opens the context menu as well as setting the shortcut to RMB. The other is that the page height of the Key Bindings page doesn't change after initialisation, so on the shorter pages there's a lot of empty space down the bottom. I don't know if this behaviour occurred before my commits, but I'll see what I can do about fixing it.

@PlutonicOverkill
Copy link
Contributor Author

Confirmed that the Key Bindings page height is still not updated when the page changes in the latest master (5e03e75).

@PlutonicOverkill
Copy link
Contributor Author

OK, I think everything works as intended now, except for the height of the Key Bindings page, which was behaving incorrectly prior to this feature. Sorry about the mess of commits, I probably should have squashed them before pushing to keep things tidier.

@akortunov
Copy link
Collaborator

There are build errors in this PR.
Enumeration in a nested name is C++11 feature.
Did you mean Qt::PreventContextMenu instead of Qt::ContextMenuPolicy::PreventContextMenu?

@PlutonicOverkill
Copy link
Contributor Author

Fixed.

@PlutonicOverkill PlutonicOverkill changed the title OpenMW-CS: Add option to reset settings to default (Feature #3275, #3682) [Incomplete] OpenMW-CS: Add option to reset settings to default (Feature #3275, #3682) May 12, 2017
@zinnschlag
Copy link
Contributor

That looks very good. There is still one issue that needs to be addressed and then we can have a merge. If you right click in the left table that also switches to the respective page. I don't think that is a right behaviour.

@PlutonicOverkill
Copy link
Contributor Author

Done.

@zinnschlag zinnschlag merged commit 5ec9781 into OpenMW:master May 12, 2017
@PlutonicOverkill PlutonicOverkill deleted the reset-category branch September 18, 2017 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants