Skip to content
This repository has been archived by the owner on May 28, 2022. It is now read-only.

ConfigTool: General Widget update #610

Closed
wants to merge 1 commit into from

Conversation

farazs
Copy link
Contributor

@farazs farazs commented Oct 4, 2014

Fix #539

Tasks:

  • Update General widget look to match mock-up
  • Implement "Help us translate" button functionality
  • Implement "Reset to defaults" button functionality
  • Implement "Clear talks" button functionality (Removed after feedback)
  • Add confirmation dialog to both(?) reset buttons
  • Add translation code for new labels

@farazs
Copy link
Contributor Author

farazs commented Oct 4, 2014

Which layout is better? Should there be spacing between the regular preferences and the "Reset" section to indicate that it's not really a setting itself?

general tab 3
general tab 2
general tab 1

@farazs
Copy link
Contributor Author

farazs commented Oct 4, 2014

Also, should both of the reset/clear buttons create confirmation dialogs before completing their action?

@farazs
Copy link
Contributor Author

farazs commented Oct 4, 2014

So I added the functionality for "Clear talks", but the problem is that the configtool doesn't have access to the talk editor to tell it to reload the database. Right now I've made it so that every time the talk editor is launched from the record tool it calls select() on the presentationModel of the talk editor. Is this too much unnecessary work if there are no changes to the talks?

An alternative would be to add a "dirty" bool to the database, which the talk editor can check every time it's show() method gets called. If the dirty is true then it can call select() to reload the presentationModel, otherwise not?

@farazs
Copy link
Contributor Author

farazs commented Oct 4, 2014

I added the necessary calls to translate() for the new text, titles etc.

What's the protocol for adding new/different text in terms of translation? Should I just add the calls to translate as I have now? Should I also populate the entries in the English translation file? Or do I have to add blank entries in the other translation files as well?

@farazs
Copy link
Contributor Author

farazs commented Oct 4, 2014

mtomwing suggested that the clear talks button should not be in the ConfigTool since we'd like to keep those separate, and then there wouldn't be a need to let the talk editor know that the talks have been updated. @dideler and @zxiiro do you agree? In that case I could just remove the button.

@zxiiro
Copy link
Member

zxiiro commented Oct 4, 2014

Yes clearing talks is a function of the talk editor not configuration.

@farazs
Copy link
Contributor Author

farazs commented Oct 4, 2014

Okay, I removed the clear talks option. I also have two options for the Reset section layout. Right now it's set to the first one. Is there anything else that needs to be done?

general option 2
general option 1

@farazs
Copy link
Contributor Author

farazs commented Oct 5, 2014

Changed the button sizes a bit and the confirmation dialog string. Screenshots:
general tab screenshot
general tab dialog

#
self.confirmResetDefaultsTitleString = self.app.translate("ConfigToolApp", "Reset to Defaults")
self.confirmResetDefaultsQuestionString = self.app.translate("ConfigToolApp",
"Are you sure you want to restore all settings to their defaults?")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All params should be aligned if you split them across multiple lines.

@farazs
Copy link
Contributor Author

farazs commented Oct 5, 2014

@mtomwing Okay, changed everything like you asked. I also went and refactored the AVWidget init so that there aren't any unnecessary class variables there either.

EDIT: There seems to be an issue with the Travis build. Looking into it now.

@mtomwing
Copy link
Member

mtomwing commented Oct 6, 2014

For whatever reason my "help us translate" button doesn't seem to have any left/right padding
screenshot 2014-10-05 17 27 37
.

@mtomwing
Copy link
Member

mtomwing commented Oct 6, 2014

Seems to be good now.
screenshot 2014-10-05 18 07 57

@farazs
Copy link
Contributor Author

farazs commented Oct 6, 2014

Merge time?

@dideler dideler added this to the 2014 Fall - UCOSP milestone Oct 6, 2014
@dideler
Copy link
Member

dideler commented Oct 6, 2014

Nice work @farazs.

I have some feedback on the reset section (may have feedback for the code as well, still need to look at it). We can make it a bit more user friendly.

To add more information before the prompt, I think changing "Reset Settings" to "Reset settings to defaults" will make users feel more comfortable pressing the button since they now have a better idea what it does (and they don't know that there is a prompt with more info).

For the prompt:

"Defaults" doesn't need to be capitalized in the window title, but if you disagree then you can leave it.

The description can be a bit more descriptive. E.g.

Your Freeseer settings will be restored to their original defaults. This will reset ..., ..., and ....

The actions can be more descriptive as well. [Reset] [Cancel] (with cancel being default)

@farazs
Copy link
Contributor Author

farazs commented Oct 6, 2014

I looked at a bunch of prompts from programs I use often ie. chrome, utorrent etc. and they all seem to follow the pattern of
Title : App name
Message : "Are you sure you want to X?", or "X will happen. Are you sure?"

I can make the title "Reset to defaults" or even "Freeseer", it shouldn't matter too much. I don't think anyone actually ever looks at it when a prompt comes up. For the message, I'm not sure if we want the user to be reading tons (or if they will even read it). Resetting settings to defaults is pretty self-explanatory imo. I could make the message something like "All your Freeseer settings will be restored to their defaults. Are you sure?". But I don't know if enumerating all the settings it will reset is helpful since we do say all. What do you think?

And I think the actions have to be from QMessageBox but I agree, it should be "Okay" and "Cancel" since that seems more appropriate for an action like this. (Nvm, just noticed there's a Reset option)

Edit: And yeah i'll change the button name to "Reset settings to defaults" so that it's more clear before the user presses it.

@farazs
Copy link
Contributor Author

farazs commented Oct 6, 2014

@dideler How about a title of "Freeseer", a message of "Your Freeseer settings will be restored to their original defaults." and "Reset" and "Cancel" buttons?

@farazs
Copy link
Contributor Author

farazs commented Oct 6, 2014

Something weird happens when I use Reset. But Yes, No, Okay and Cancel all work fine. Here's what happens:
okay prompt
reset prompt

Improved the overall layout of the General tab.
Added "Reset Settings" and "Help us translate"
buttons. Some refactoring to remove unnecessary
instance variables.
@mtomwing
Copy link
Member

mtomwing commented Oct 7, 2014

Merged in 19f959d. I ended up going with the Reset/Cancel button combo.

@mtomwing
Copy link
Member

mtomwing commented Oct 7, 2014

Thanks for your contribution!

@mtomwing mtomwing closed this Oct 7, 2014
@dideler
Copy link
Member

dideler commented Oct 7, 2014

I looked at a bunch of prompts from programs I use often ie. chrome, utorrent etc. and they all seem to follow the pattern of
Title : App name
Message : "Are you sure you want to X?", or "X will happen. Are you sure?"

This doesn't seem to be the case when I check Chrome.

image

it should be "Okay" and "Cancel" since that seems more appropriate for an action like this.

A UX rule of thumb is to stay away from button labels like "Yes", "No", "OK". You should try to be specific (e.g. a call to action) yet keep it concise. Reading the button should remind the user what action will take place. See http://ux.stackexchange.com/questions/9946/should-i-use-yes-no-or-ok-cancel-on-my-message-box (or any search about button labels WRT to UX will bring up similar results).

But I don't know if enumerating all the settings it will reset is helpful since we do say all. What do you think?

I do think saying what will be reset is useful, but in this case I've changed my mind because I think there would be too many things to mention. If there were only a few settings or we could word it so that it's not a lot of text then the extra information would help, but currently we risk putting too much text which dilutes the message.

@farazs
Copy link
Contributor Author

farazs commented Oct 8, 2014

The dialogs I checked were things like opening all bookmarks and stuff like that. In my opinion, something like resetting all settings is a way bigger deal in Chrome than in Freeseer. So if a user really wants to reset all their settings in Chrome they should be informed about all the changes that will occur but I don't think the same should be true for Freeseer. Recovering from an unintentional reset in Chrome would take a lot longer than in Freeseer.

Yeah I agree that having "Reset" would be better than "Okay" if all else were equal. But the user expects dialogs to have both options on the right hand side, and when the Reset option is set, it goes all the way to the left hand size (as shown in the screenshots in my earlier post). Imo the downside of the unintuitive layout is greater than the upside of the better wording.

But it's upto you guys in the end and I think you ended up merging with "Reset" instead of "Okay" which is fine too.

@farazs farazs deleted the update-configtool-general branch October 8, 2014 02:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update General Tab of ConfigTool
4 participants