GNOME Bugzilla – Bug 608108
unsaved changes dialog should be more informative
Last modified: 2012-02-02 16:23:01 UTC
A nitpick, really, but the "do you want to quit? you have some unsaved changes" dialog is not as helpful as it could be. It could, for example, say the time at which the file was last saved, so the user knows for how long he has been working on the file since the last save, and if he wants to discard the changes.
Created attachment 186373 [details] [review] Patch to show user the time of previous modification
Created attachment 186385 [details] [review] Removed merge-cache changes
Review of attachment 186385 [details] [review]: There are localization/translation problems with this implementation. You can't split strings around as individually translatable strings, this will be incompatible with various languages/locales. Also, if you are dealing with plural strings, you need to use ngettext (commit eee36cc8152... could serve as an example). Whatsmore, while your way of calculating time deltas might be "technically" correct, it is incorrect for users. Instead of checking the file modification time on the filesystem, you need to check for the last time the file was actually opened or saved by pitivi. For example, I opened an existing project, did a quick change, triggered the dialog, and it told me that my changes since the "last 113 days" would be lost.
Created attachment 186485 [details] [review] Displays time since project was loaded Does not show time if project hasn't been saved before
Review of attachment 186485 [details] [review]: Rather than comparing the last modification time of the project file with the current time, it would be cleaner IMO to store project.first_unsaved_change_time, and compare the current time with that. The first_unsaved_change_time attribute is set to None when a ProjectManager is created, and when the project is saved, and set to current time (if it's == None) in ProjectManager.projectChangedCb(). All of these messages should be complete sentences: time_since = _(" since the last %s hours" % timeSinceChanged.seconds/3600) time_since = _(" since the last %s seconds" % timeSinceChanged.seconds) time_since= _(" since the last %s minutes, %s seconds" % (timeSinceChanged.seconds/60 , timeSinceChanged.seconds%60)) Please add a unittest! Thanks! Some more suggestions: - Please remove: #Bug Fix 608108 - More information in saved dialog box - Please use time_saved_delta: timeSavedDelta = time_now - time_mod - Please separate on two lines (or more): if not project.uri: return "" else: timeSinceChanged = timeSavedDelta else: time_since= _(" since the last %s minutes, %s seconds" % (timeSinceChanged.seconds/60 , timeSinceChanged.seconds%60)) - Add exactly ONE space after each comma: def _getChangesWillBeLostWarning(self,project, time_loaded): - There must be NO space before a comma: else: time_since= _(" since the last %s minutes, %s seconds" % (timeSinceChanged.seconds/60 , timeSinceChanged.seconds%60)) - At the left and at the right of each operator must be exactly ONE space: else: time_since= _(" since the last %s minutes, %s seconds" % (timeSinceChanged.seconds/60 , timeSinceChanged.seconds%60))
I now have a more efficient implementation in my "restore-from-backup" branch.
commit 1214d79b4f05bc806b5e454da66684a0495946f2 Author: Jean-François Fortin Tam <nekohayo@gmail.com> Date: Sun Jan 22 21:11:11 2012 -0500 Show the time for the unsaved changes confirmation dialog Fixes bug #608108 commit 9a20ea4fdc81f125e315c20a1a5b495eff3c1ead Author: Jean-François Fortin Tam <nekohayo@gmail.com> Date: Sun Jan 22 18:08:39 2012 -0500 utils: Allow showing human-readable time deltas Make beautify_ETA more robust