Code review comment for lp:~artem-anufrij/scratch/dont-force-to-save-unsaved

Revision history for this message
Robert Roth (evfool) wrote :

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)

« Back to merge proposal