#501 - Prompt user to save/discard/cancel unsaved changes when switching from one talk to another in Talk Editor #605

benbuckley · 2014-10-01T01:04:24Z

Ultimately, I would like to have it set up so that, when the user has unsaved changes in a talk and clicks on a different talk from the tableview, a window comes up asking them if they want to save their changes, with the options "Save" (save changes and select the new talk), "Discard" (don't save changes, just select the new talk) and "Cancel" (don't save changes but don't select the new talk either).

I have not made any huge changes yet -- a lot of my time has been spent just understanding Qt and how all the methods interact with each other. I expect to make more progress this week. However, to start with, I have added a method called "click_talk" which mediates between tableView and talk_selected. I know it doesn't look very useful right now, but I believe that it will make things easier in the long run to have a method that acts only when the user clicks on the talk -- as it is, talk_selected is called both when the user clicks on a talk, and in the select_talk method.

ToDo:

Add QMessageBox with save/discard/cancel options
Add functionality to this QMessageBox (Will attach screenshot soon)
Ensure that NEW talk is selected after user selects "Save"
Change options in QMessageBox to "Yes", "No" and "Cancel"
Enable user to save changes to current talk before creating new talk (Perhaps this should be made a separate issue -- thoughts?
Prompt user to Continue Editing, Discard Changes, or Save Changes when closing Talk Window as well.

Screenshot of QMessageBox:

Closes Issue #501 when done.

benbuckley · 2014-10-02T03:38:42Z

The edits just barely do as promised -- when the user selects a new talk when the current one has unsaved changes, a window comes up with the options "save/discard/cancel" where each option essentially does what it says.

However, I am not done yet. For one thing, I would like to see if I can make my code a bit prettier. For another thing, I would like to see if I can make it so that, when the user creates a new talk (which by default becomes highlighted on the list) they have the option of saving the details on their current talk.

In addition: currently, if you select "Save" from the save/discard/cancel window, your current talk remains selected and highlighted. I would like for the NEW talk to be selected and highlighted after the changes are made.

Also, by default, the "Discard" button says "Close without saving" which is a little odd. Maybe I should change it to yes/no/cancel instead of save/discard/cancel?

dideler · 2014-10-02T10:34:16Z

To make it easier for us to see what you did and what you plan to do, please create a task list in the PR description. Keep the items in the list concise, so they're easy for others to read.

At the end of your PR description, please reference the related issue (e.g. Closes #...).

Since this PR deals with UI changes, please show us screenshots of the UI whenever you make significant changes. See the screenshot below for how to add images.

I'll take a look at the code and play around with the changes later today, so I can give more feedback then.

With these changes, the user is warned when they select an existing talk or create a new talk when the talk they're currently editing has unsaved changes. Fix Freeseer#501 Close Freeseer#605

benbuckley · 2014-10-04T19:55:26Z

This looks fine to me. Are there any further changes I should make? Also, in general, should I squash it all into one big commit?

mtomwing · 2014-10-05T18:40:17Z

src/freeseer/frontend/talkeditor/talkeditor.py

@@ -28,6 +28,7 @@
 from PyQt4.QtCore import SIGNAL
 from PyQt4.QtCore import QStringList
 from PyQt4.QtCore import Qt
+from PyQt4.QtCore import QPersistentModelIndex


Imports should be sorted (easy if your editor supports inline sorting).

zxiiro · 2014-10-05T19:07:33Z

Sorry it's not clear to me what the difference between No and Cancel are?

Edit: To clarify my confusion, what's the purpose of selecting a new talk if we are discarding it vs not selecting it if we cancel?

benbuckley · 2014-10-05T19:23:22Z

zxiiro: Suppose you have made changes to a talk, and then click on another talk from the list.

If you select "No", then your unsaved changes are lost, and the TalkDetailsWidget is repopulated with the details of the new talk. It's a way for the user to confirm "That's right, I don't want to keep the changes I made."

If you select Cancel, then your unsaved changes stay in the TalkDetailsWidget, but they are not saved -- it's as though you never clicked on the new talk to begin with. It's a way for the user to say "On second thought, I'm not sure if I want to save or discard my changes yet, and I need to think about this some more."

mtomwing, I have noted your comments and will address them in due time.

zxiiro · 2014-10-05T19:41:39Z

@benbuckley makes sense. thanks for the clarification.

benbuckley · 2014-10-06T00:26:45Z

src/freeseer/frontend/talkeditor/talkeditor.py

    def talk_selected(self, model):
        self.mapper.setCurrentIndex(model.row())
        self.talkDetailsWidget.enable_input_fields()
        self.talkDetailsWidget.saveButton.setEnabled(False)
+        self.currentTalkIndex = QPersistentModelIndex(model)


It needs to reflect the talk most recently selected by the user, so it's not obvious to me why putting it in load_presentations_model would be helpful -- what did you have in mind?

Based on my new understanding of QPersistentModelIndex, it makes sense for it to be here.

When the user switches from one talk to another or creates a new talk when the currently selected talk has unsaved changes, a window comes up asking the user if they wish to save their changes, with the options "Yes", "No", and "Cancel". "Yes" saves the changes and selects the new talk; "No" selects the new talk without saving the changes; and "Cancel" keeps the current talk selected without discarding or saving any changes made. Fix Freeseer#501 Close Freeseer#605

benbuckley · 2014-10-06T02:59:16Z

I've resorted the import statements, and put the QMessageBox stuff on one line. How does it look now?

mtomwing · 2014-10-06T20:10:29Z

src/freeseer/frontend/talkeditor/talkeditor.py

+        return confirm
+
+    def click_talk(self, model):
+        log.info("User selected row " + str(model.row()))


log actually works like printf when it comes to string construction.

log.info('hello world %s %d', 'some string', 10) => 'hello world some string 10'

When the user switches from one talk to another or creates a new talk when the currently selected talk has unsaved changes, a window comes up asking the user if they wish to save their changes, with the options "Yes", "No", and "Cancel". "Yes" saves the changes and selects the new talk; "No" selects the new talk without saving the changes; and "Cancel" keeps the current talk selected without discarding or saving any changes made. Fix Freeseer#501 Close Freeseer#605

mtomwing · 2014-10-07T10:28:09Z

@dideler or @zxiiro, take a look at this when you have a chance since you're more familiar with the talk editor. Should be good to merge if you don't have any objections.

dideler · 2014-10-07T18:00:17Z

@benbuckley not sure why you changed the button text? See #610 (comment)

Regarding the three options, I think this overcomplicates the prompt. It should just be "Save changes" (default option) and "Discard changes".

After the action is completed in both cases, the user would be brought to the other talk they selected—that was their intention after all.

If they misclicked (the odds are low, but possible), then they still don't need a cancel option. Why? Because they can choose the save option and go back to the previously selected talk.

benbuckley · 2014-10-07T18:35:46Z

Re: Avoiding "Yes" and "No" -- I originally used QMessageBox.Save and QMessageBox.Discard. When I ran it this way, the Discard button would default to the text "Close Without Saving", which seemed misleading, since it doesn't close the whole program, it just switches from one talk to another. I figured maybe it showed up differently on different computers, and I reasoned that if I just went with "Yes" and "No", it would be more consistent from one machine to another. However, I'm not married to the idea, and I can change it back to Discard and Save.

I would like to defend the "Cancel" option, though.

We have accepted the premise that it's possible for a user to misclick -- there are a variety of reasons why this might happen (maybe their elbow slipped, maybe they're just very indecisive). Suppose we only have "Save" and "Discard" options. Then if the user innocently misclicks, we're forcing them to either lose all their changes, or commit to all their changes.

I can think of plenty of times I've made big changes to a file and been unsure whether I wanted to keep them that way or not. Being forced to either Save or Discard them would put me in a bind. If I Save, I can't go back to how the file was before I made my changes if my changes turn out to be bad. If I Discard, then I lose all my changes and HAVE to start over. With the "Cancel" button, the user still has the option of saving or discarding their changes later if they wish.

When the user switches from one talk to another or creates a new talk when the currently selected talk has unsaved changes, a window comes up asking the user if they wish to save their changes, with the options "Yes", "No", and "Cancel". "Yes" saves the changes and selects the new talk; "No" selects the new talk without saving the changes; and "Cancel" keeps the current talk selected without discarding or saving any changes made. Fix Freeseer#501 Close Freeseer#605

dideler · 2014-10-07T19:04:37Z

I'm not sure I understand your argument for keeping the cancel option.

We have accepted the premise that it's possible for a user to misclick

That's right, but the odds are low and the mistake is easy to recover from with save and discard options. The extra complexity from having more options actually complicates the decision the user has to make. Not to mention that "Cancel" and "No/Discard" are similar enough (both can be thought of as negatives) to make the process even more confusing.

If I Save, I can't go back to how the file was before I made my changes if my changes turn out to be bad.

I don't follow. Why can't you go back? If you weren't done editing the talk but you accidentally clicked another talk, then you save changes and go back to the talk and continue from where you left off.

If I Discard, then I lose all my changes and HAVE to start over.

Start over what process exactly? Either you want the changes you made (or some variation of them) or you don't want them at all. In the former case, you save and go back to the talk. In the latter case you discard changes and focus on the new talk you selected.

dideler · 2014-10-07T19:30:58Z

FWIW, I just performed a guerilla user test using this sketch:

I described the top window (i.e. talk editor UI - it looks nothing like Freeseer's, but it doesn't need to) and described the situation: You're editing a talk and then select a new talk before saving your changes. You're then presented with a prompt.

Both prompts were initially hidden, I then revealed the first prompt and asked for their thoughts.

The response was something along the lines of "If I say yes, is it going to save to the new talk? If I say no, is it going to give me another option to save to the old talk or do I lose everything? And if I press cancel, is it going to not save anything or go back to the old talk?"

So there was some confusion. (Note that I didn't give any hints or mention my own opinion.)

I then revealed the second prompt and again asked for their opinion.

The response was something like "It's a clear question about what I forgot to do and clear where it will save the changes to."

I then asked about the situation where you accidentally misclicked a talk. The response was something like "I would rarely do that. But if I did, I would save changes and go back. But with the other one it's unclear what I have to click to go back. Do I click no or cancel?"

Now this was just a sample size of one, which is statistically insignificant, but you get the idea.

benbuckley · 2014-10-07T20:06:53Z

My problem is that saving is not a neutral decision. Saving a set of bad changes is as bad as discarding a set of good changes, and I wouldn't want to do either one of them by accident, or by force.

Suppose the options are Save and Discard. If I'm in the middle of doing some major rewrites on a description, I might not know yet whether I want to keep my changes or not. If I already knew with complete certainty that I wanted to keep my changes, I would have clicked "Save Talk" already. If I already knew that I wanted to get rid of the changes, I would have exited the talk editor or discarded the changes some other way. When I'm still in the process of editing, I'm in a state of uncertainty.

If you want to reduce the number of buttons, how about something like this? Feel free to suggest other ways to word the query:

In the likely situation that the person intended to click on the other talk, then it's obvious which button to press if they want to discard the changes to the current talk. If they clicked by accident, or if they just forgot to save their current talk, then they can cancel and continue editing their current talk or save it if they wish. Admittedly the word "Cancel" is still a tad ambiguous, but it's at least obvious that it's not the same thing as "Discard".

zxiiro · 2014-10-07T20:10:31Z

I like Dennis' suggestion because his window informs the user what is happening "The talk you were editing has unsaved changes" and then provides 2 possible suggestions on what can be done. I feel its' not ambiguous and is in a language natural to an end user.

Simply stating "Discard changes?" is a bit ambiguous to me, what changes are being discarded? ... Settings? Talk Data?

dideler · 2014-10-08T11:10:36Z

The "Discard changes?" is nudging users towards discarding changes, which might be improbable assuming they want their changes saved most of the time. This is why I think it's best to have a neutral message and have the buttons contain the actions (which also has the benefit of reduced mistakes since users sometimes skip the message and only read the buttons).

After thinking about it some more, I think this is a fair compromise

+-----------------------------------------------------+
| The talk you were editing has unsaved changes.      |
|                                                     |
| [Continue editing] [Discard changes] [Save changes] |
+-----------------------------------------------------+

It has all three actions. We assume saving is the most common action, so it's primary. Cancelling navigation is the least likely (only happens on misclicks) so it's on the far left.

Other wording choices for "Continue editing"

"Cancel navigation"
- Note that just "Cancel" would not be sufficient since it would lead to confusion. The message is about unsaved changes, but the Cancel function cancels navigation.
"Go back" or "Go back to editing talk"
"Stay" or "Stay on talk"

P.S. I think autosave would be best from a UX perspective, but I think we experimented with that before and there was some issue, can't remember what.

benbuckley · 2014-10-09T22:16:19Z

I like it. I'll implement it as soon as possible, which will probably mean after Thanksgiving.

It is a little tricky to customize the button text (up until now, I've just been using the defaults, i.e. "Save", "Yes", "No", "Cancel", "Discard"), but I think I have an idea of how to do it.

dideler · 2014-10-10T10:39:45Z

@benbuckley there was a PR by @farazs recently that had customized text, maybe that'll help. Search for "reset" https://github.com/Freeseer/freeseer/pull/610/files

When the user switches from one talk to another or creates a new talk when the currently selected talk has unsaved changes, a window comes up asking the user if they wish to save their changes, with the options "Yes", "No", and "Cancel". "Yes" saves the changes and selects the new talk; "No" selects the new talk without saving the changes; and "Cancel" keeps the current talk selected without discarding or saving any changes made. Fix Freeseer#501 Close Freeseer#605

dideler · 2014-10-16T10:14:39Z

@benbuckley please ping us on the PR when you respond to feedback / push new commits. Just say something like "fixed" or "please review" so we get a notification. Otherwise the PR will be ignored since we don't know that there's been activity.

dideler · 2014-10-16T10:35:26Z

Found a bug. If you try to exit the talk editor with unsaved changes and select "Discard changes", it actually saves your changes.

dideler · 2014-10-16T10:40:20Z

src/freeseer/frontend/talkeditor/talkeditor.py

@@ -283,10 +297,51 @@ def search_talks(self):
        self.proxy.setFilterKeyColumn(-1)
        self.proxy.setFilterFixedString(self.commandButtons.searchLineEdit.text())

+    def show_save_prompt(self):


Please add docstrings for the new methods you wrote. See some of the existing docstrings in this file to get an idea of how they should look. Keep them concise.

benbuckley · 2014-10-17T05:54:06Z

I have made most of the changes you suggested. However, for some reason, using setModal(True) and show() seems to give strange results for me -- the buttons in the message box don't always do the things they're suppose to do.

When you say "ping", does that mean leaving a comment (possibly including a tag like @dideler)?

dideler · 2014-10-17T09:09:28Z

I have made most of the changes you suggested. However, for some reason, using setModal(True) and show() seems to give strange results for me -- the buttons in the message box don't always do the things they're suppose to do.

Okay thanks. Let's keep it like how you had it originally.

When you say "ping", does that mean leaving a comment (possibly including a tag like @dideler)?

Yup. That way we get notified and can take another look at the PR.

If you squash your commits into a single commit, we can get this merged.

After that, will you be working on #532, #541, or tests for #605?

When the user switches from one talk to another or creates a new talk when the currently selected talk has unsaved changes, a window comes up asking the user if they wish to save their changes, with the options "Yes", "No", and "Cancel". "Yes" saves the changes and selects the new talk; "No" selects the new talk without saving the changes; and "Cancel" keeps the current talk selected without discarding or saving any changes made. Fix Freeseer#501 Close Freeseer#605

benbuckley · 2014-10-17T19:14:17Z

@dideler I've squashed the commits. I think moving on to #532 would be a good idea, since I have TalkEditor.py fresh in my mind.

EDIT: Actually, now that you mention it, I've been thinking I should learn a bit about how unit tests work, and this seems like a good opportunity to learn how. I'm interested in doing tests for #605

dideler · 2014-10-18T13:30:22Z

@benbuckley I noticed the commit message contains outdated information. Can you update it?

When the user switches from one talk to another, exits the talk editor, or creates a new talk when the currently selected talk has unsaved changes, a window comes up warning the user that there are unsaved changes. The options are "Save Changes", "Discard Changes", and "Continue Editing". Fix Freeseer#501 Close Freeseer#605

benbuckley · 2014-10-18T20:26:51Z

@dideler The old commit message still said the options were "Yes", "No" and "Cancel". I have changed it to reflect the new buttons, "Save Changes", "Discard Changes" and "Continue Editing". Is that what you were talking about?

dideler · 2014-10-18T20:39:29Z

Looks good 👍

Thanks for your contribution!

benbuckley changed the title ~~#501 - Prompt user to save/discard/cancel unsaved changes when switching from one talk to another in Talk Editor~~ WIP: #501 - Prompt user to save/discard/cancel unsaved changes when switching from one talk to another in Talk Editor Oct 1, 2014

dideler added this to the 2014 Fall - UCOSP milestone Oct 1, 2014

benbuckley force-pushed the 501-talk-prompt branch 2 times, most recently from 43aa91d to a5e43d5 Compare October 2, 2014 03:19

benbuckley force-pushed the 501-talk-prompt branch 2 times, most recently from 36c06a3 to e3bf341 Compare October 4, 2014 19:45

benbuckley changed the title ~~WIP: #501 - Prompt user to save/discard/cancel unsaved changes when switching from one talk to another in Talk Editor~~ #501 - Prompt user to save/discard/cancel unsaved changes when switching from one talk to another in Talk Editor Oct 4, 2014

mtomwing reviewed Oct 5, 2014
View reviewed changes

benbuckley reviewed Oct 6, 2014
View reviewed changes

benbuckley force-pushed the 501-talk-prompt branch from e3bf341 to 62bf1c3 Compare October 6, 2014 02:53

mtomwing reviewed Oct 6, 2014
View reviewed changes

benbuckley force-pushed the 501-talk-prompt branch from 62bf1c3 to 85d651f Compare October 6, 2014 20:16

benbuckley force-pushed the 501-talk-prompt branch from 85d651f to 632af20 Compare October 6, 2014 20:30

benbuckley force-pushed the 501-talk-prompt branch from 632af20 to 20ccebc Compare October 7, 2014 18:45

benbuckley force-pushed the 501-talk-prompt branch from 20ccebc to 82e4687 Compare October 16, 2014 03:03

dideler reviewed Oct 16, 2014
View reviewed changes

benbuckley force-pushed the 501-talk-prompt branch from 82e4687 to 45f4466 Compare October 17, 2014 05:29

benbuckley force-pushed the 501-talk-prompt branch from 1d53153 to c85cfa4 Compare October 17, 2014 19:06

benbuckley force-pushed the 501-talk-prompt branch from c85cfa4 to 130bf1e Compare October 18, 2014 20:22

dideler merged commit 130bf1e into Freeseer:master Oct 18, 2014

This was referenced Oct 18, 2014

Tests for talkeditor.py following PR #605 #630

Open

WIP: #630: Create tests for talkeditor.py following PR #605 #631

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#501 - Prompt user to save/discard/cancel unsaved changes when switching from one talk to another in Talk Editor #605

#501 - Prompt user to save/discard/cancel unsaved changes when switching from one talk to another in Talk Editor #605

benbuckley commented Oct 1, 2014

benbuckley commented Oct 2, 2014

dideler commented Oct 2, 2014

benbuckley commented Oct 4, 2014

mtomwing Oct 5, 2014

zxiiro commented Oct 5, 2014

benbuckley commented Oct 5, 2014

zxiiro commented Oct 5, 2014

benbuckley Oct 6, 2014

mtomwing Oct 6, 2014

benbuckley commented Oct 6, 2014

mtomwing Oct 6, 2014

mtomwing commented Oct 7, 2014

dideler commented Oct 7, 2014

benbuckley commented Oct 7, 2014

dideler commented Oct 7, 2014

dideler commented Oct 7, 2014

benbuckley commented Oct 7, 2014

zxiiro commented Oct 7, 2014

dideler commented Oct 8, 2014

benbuckley commented Oct 9, 2014

dideler commented Oct 10, 2014

dideler commented Oct 16, 2014

dideler commented Oct 16, 2014

dideler Oct 16, 2014

benbuckley commented Oct 17, 2014

dideler commented Oct 17, 2014

benbuckley commented Oct 17, 2014

dideler commented Oct 18, 2014

benbuckley commented Oct 18, 2014

dideler commented Oct 18, 2014

#501 - Prompt user to save/discard/cancel unsaved changes when switching from one talk to another in Talk Editor #605

#501 - Prompt user to save/discard/cancel unsaved changes when switching from one talk to another in Talk Editor #605

Conversation

benbuckley commented Oct 1, 2014

benbuckley commented Oct 2, 2014

dideler commented Oct 2, 2014

benbuckley commented Oct 4, 2014

mtomwing Oct 5, 2014

Choose a reason for hiding this comment

zxiiro commented Oct 5, 2014

benbuckley commented Oct 5, 2014

zxiiro commented Oct 5, 2014

benbuckley Oct 6, 2014

Choose a reason for hiding this comment

mtomwing Oct 6, 2014

Choose a reason for hiding this comment

benbuckley commented Oct 6, 2014

mtomwing Oct 6, 2014

Choose a reason for hiding this comment

mtomwing commented Oct 7, 2014

dideler commented Oct 7, 2014

benbuckley commented Oct 7, 2014

dideler commented Oct 7, 2014

dideler commented Oct 7, 2014

benbuckley commented Oct 7, 2014

zxiiro commented Oct 7, 2014

dideler commented Oct 8, 2014

benbuckley commented Oct 9, 2014

dideler commented Oct 10, 2014

dideler commented Oct 16, 2014

dideler commented Oct 16, 2014

dideler Oct 16, 2014

Choose a reason for hiding this comment

benbuckley commented Oct 17, 2014

dideler commented Oct 17, 2014

benbuckley commented Oct 17, 2014

dideler commented Oct 18, 2014

benbuckley commented Oct 18, 2014

dideler commented Oct 18, 2014