Scratch

Merge lp://staging/~artem-anufrij/scratch/dont-force-to-save-unsaved into lp://staging/~elementary-apps/scratch/scratch

dont-force-to-save-unsaved
Merge into scratch

Proposed by Artem Anufrij on 2014-10-04

Status:

Merged

Approved by:

Robert Roth on 2014-10-19

Approved revision:

1414

Merged at revision:

1393

Proposed branch:

lp://staging/~artem-anufrij/scratch/dont-force-to-save-unsaved

Merge into:

lp://staging/~elementary-apps/scratch/scratch

Diff against target:

746 lines (+236/-186)

7 files modified

HACKING (+1/-1)
src/Dialogs/PreferencesDialog.vala (+1/-5)
src/MainWindow.vala (+53/-6)
src/Scratch.vala (+16/-2)
src/Services/Document.vala (+145/-161)
src/Widgets/DocumentView.vala (+13/-3)
src/Widgets/ToolBar.vala (+7/-8)

To merge this branch:

bzr merge lp://staging/~artem-anufrij/scratch/dont-force-to-save-unsaved

Related bugs:

Bug #1355040: Scratch tries to save blank file after entry and deletion	Medium	Fix Released
Bug #1362411: Every ten seconds worth of editing a new file it shows a Save File dialog.	Medium	Fix Released

Link a bug report

Related blueprints:

Scratch shouldn't force the user to save unsaved documents on exit (Low)

Default location for saving (Low)

Reviewer	Review Type	Date Requested	Status
Robert Roth (community)	code review	2014-10-04	Approve on 2014-10-19
Danielle Foré	ux		Approve on 2014-10-18
Review via email: mp+237154@code.staging.launchpad.net

Commit message

Use temporary files for unsaved files to be able to restore them at next startup

Description of the change

BLUEPRINT DESCRIPTION:
+1 for keeping in .local/share/scratch/unsaved/. Because losing work is the worst user experience ever. If we keep files in /tmp they can get deleted by the system (think sudden power outage and user boots again or even someone is keeping /tmp in RAM - other systems and maybe future eOS). And if we keep them in trash user can delete them unknowingly because he expects that only files that he himself moved to trash are there. Also files that are not saved anywhere IMHO should have a *highly visible* warning so that even user that is absolutely new to PCs knows it's not saved. (Perhaps a red exclamation mark on the tab? Or the entire tab tinted slightly red?) Won't it be annoying? No, because how often do you write and keep in editor (for a longer time) something that you don't want to save? ~grzesiek1e5

lp://staging/~artem-anufrij/scratch/dont-force-to-save-unsaved updated on 2014-10-04

1399. By artem-anufrij on 2014-10-04: Ready for check (clear 'file != null' block)
1400. By artem-anufrij on 2014-10-04: Replace 'scratch' to exec_name. Fix code style.
1401. By artem-anufrij on 2014-10-04: Delete old backup file after 'save as...'

Revision history for this message

Robert Roth (evfool) wrote on 2014-10-11:

Code looks mostly OK, here are some notes:
* Singleton is not needed here, accessing data_home_folder_unsaved seems to be the only reason you're making the Scratch instance a singleton, but that variable should be part of settings (e.g I want to configure that in dconf - or scratch Preferences if Dan allows - to use /tmp instead of my home dir)
* string message = _("File ") + " \"<b>%s</b>\" ".printf (get_basename ()) + _("was modified by an external application. Do you want to load it again or continue your editing?");
This is not translation-friendly, and there are more like this. Translatable strings should not be concatenated.
It can also be written like: _("File %s was modified by an external application.").printf ("<b>%s</b>".printf (get_basename ())).
I see that there are some of these coming from before your commits, so it's not 100% your code, but they should be fixed too (not necessarily on this branch), but at least the code we touch should get better after each touch.
* Add a description and a commit message to the merge proposal
I haven't tested this yet, this is just a code review, will test later, with the fixes.

review: Needs Fixing (code review)

lp://staging/~artem-anufrij/scratch/dont-force-to-save-unsaved updated on 2014-10-11

1402. By artem-anufrij on 2014-10-11: dev-commit
1403. By artem-anufrij on 2014-10-11: get_user_cache_dir() for termporary files; changed the text.
1404. By artem-anufrij on 2014-10-11: always open temporary files on startup

Revision history for this message

Artem Anufrij (artem-anufrij) wrote on 2014-10-13:

Please check the fixes.
- temporary location: .local/share/scretch-text-editor/unsaved
- text style was fixed.

Revision history for this message

Robert Roth (evfool) wrote on 2014-10-13:

See my inline comments. I'm still not sure the scratch app instance should be made singleton just to be able to access the path variable, that's a second reason why settings-based path was suggested, as the settings object is available globally.

review: Needs Fixing

lp://staging/~artem-anufrij/scratch/dont-force-to-save-unsaved updated on 2014-10-13

1405. By artem-anufrij on 2014-10-13: code fixing

Revision history for this message

Robert Roth (evfool) wrote on 2014-10-13:

Ok, looks nice, as per Slack comments the path doesn't have to be configurable, so we're keeping the app instance singleton. Approved.

review: Approve

lp://staging/~artem-anufrij/scratch/dont-force-to-save-unsaved updated on 2014-10-13

1406. By artem-anufrij on 2014-10-13: temporary typer

Revision history for this message

Danielle Foré (danrabbit) wrote on 2014-10-14:

I think if you close the individual tab (as opposed to the whole app) you should be prompted to either save or delete. I don't think we should encourage a workflow where users can perpetually store a bunch of documents in a hidden folder. Additionally, in the current workflow I'm not sure how I'm supposed to discard a document that I've decided I don't actually want to save.

I'm also thinking that we should override the tab/title name for unsaved documents and just call them "unsaved document" or something instead of referring to them as a file name with a location inside the app.

review: Needs Fixing (ux)

lp://staging/~artem-anufrij/scratch/dont-force-to-save-unsaved updated on 2014-10-15