GNOME Bugzilla – Bug 712192
file-export update
Last modified: 2013-12-08 15:23:49 UTC
Created attachment 259709 [details] [review] Patch Hi, following up on the discussion on the dev ml where Peter Sikking had some update propositions. I acted up on them, and implemented, with some additional propositions to be reviewed. Peter Sikking's message: https://mail.gnome.org/archives/gimp-developer-list/2013-November/msg00026.html Extract of relevant part: --------- reviewing the situation, I see that the straightforward solution is to relabel Export... -> Export As... (in all states) “Export to” -> Export (when no export target) you can see that this achieves perfect mirroring of Save and Save As... “Export to foo.xyz” and “Overwrite foo.xyz” stay as-is, they work well with Export As... ---------- So the attached patch made the change. Since now we say "Export As", I thought that “Export to foo.xyz” should rather be “Export as foo.xyz”. If that was really a mistake (especially with the last line of Peter's saying), well easy to change back. Just tell me. The patch makes: "Export" -> "Export As..." (all states) "Export to" -> "Export" (no export target) or "Export as foo.png" (previously exported to foo.png) Also I noticed that "Save" was always just "Save". So I update it to "Save as foo.xcf" when the file has already been saved (mimick Export behavior). Finally I also update the action's names to file-export and file-export-as (mimmick file-save and file-save-as). Of course, to not mess up user's shortcuts, I migrate their old shortcut when migrating their menurc to the new naming (with the migration system I wrote earlier).
Maybe I'm wrong, but it looks like the migration stuff in here is broken if it is run twice. <Actions>/file/file-export-to is converted, on the first run, to <Actions>/file/file-export. The next migration would change <Actions>/file/file-export to <Actions>/file/file-export-as. Therefore, <Actions>/file/file-export-to becomes <Actions>/file/file-export-as when the migration is run the second time, which is not the intended effect. Also, most of the migration stuff has testcases in app/tests. Adding a testcase for menurc migration might be appropriate.
Hi, By definition, it can be run only once. How would it be possible to run the migration twice? If such a thing happens, that's a definite bug, because it means GIMP could overwrite your new settings!
Also, even though it did happen (note that I never saw such a case, but I assume), what you say would not happen. I remind we don't modify files inplace in the config migration. We copy them (and the copy may be updated to new type of settings). So *if* the migration was to be run twice, we would still end up with the right action naming (there would still be user setting data loss, of any settings made *after* migration, so that's a bad bug. But not the one you talk about).
Suppose a user upgrades from 2.8 to 2.10. The migration is run. Now, suppose gimp 3.0 is released, and the user upgrades from 2.10 to 3.0. The migration will be run again. Right?
As answered on IRC: 16:57 < Jehan_ZeMarmot> drawoc: this is the code for 2.10 with specific rules. The rules for 3.0 will be different, and so on. 16:58 < Jehan_ZeMarmot> By definition of the config files migration, as it evolves, the rules evolve too. 16:59 < Jehan_ZeMarmot> For instance right now, we don't migrate menurc from before 2.0. 16:59 < Jehan_ZeMarmot> Every version may have particularities.
Sorry Jehan, no. you change too much and I must say, with devastating effect. first of all I did my job and updated the spec: <http://gui.gimp.org/index.php/Save_%2B_export_specification> checked 98 occurrences of “export” (twice) and updated to the new situation. in a nutshell, all it is is a string change. Export... -> Export As... (no other changes to this menu item or the code behind it. “Export to” -> Export (when no export target) (this is the menu item where “Export to foo.xyz” and “Overwrite foo.xyz” are swapped in as appropriate) do not touch the Save menu items, they work fine, as-is, since 1984. Save is for the main window content and the name of that is in the title bar of that main window. and that is it.
Sent by email: Ok. I'll simplify the patch. Should I change the action's names to follow their label? In particular "Export" would correspond to "file-export" and "Export As..." corresponds to "file-export-as" (otherwise we would end up in the strange situation where "Export" <=> "file-export-to" and "Export As..." <=> "file-export"). Or you care only about UI, not action names, and I should ask Mitch for this?
Yes, the action names and internal enum names should also change, or we have a lot a confusion. The functionality doesn't change anyway.
Created attachment 259728 [details] [review] Updated patch. Just export labels and actions renamed. Hop new patch!
Jehan, since your proposed fix is to only do this new migration step for config files that are migrated from 2.8 (I think that's what you're suggesting), do you think you could add that check to the code now? It would make me feel better, and I don't see any reason not to.
Were the actions "file-export" and "file-export-to" only available since 2.8? I guess so since the save/export change appeared in 2.8, but just to be sure.
It looks like they were introduced in this commit: https://git.gnome.org/browse/gimp/commit/?id=c1a226bc749dea0d9a59fe01e0e5d78a7efd5166 which does not appear to be in 2.6. 2.7 (development release) also had these actions though. So, I suppose the filter should be applied to 2.7/2.8. Really, you could just make it apply to anything <= 2.8, if that's simpler.
Created attachment 259777 [details] [review] New patch. Yeah I just checkout the gimp-2-6 branch and grep-ed "file-export". No result. As for 2.7, it does not matter. That was a dev branch. We don't care about dev branch's config files and never migrate them. What was in dev stays in dev. :-) See the migration code: for (i = (GIMP_MINOR_VERSION & ~1); i >= 0; i -= 2) Attached updated patch with version check.
Looks good to me. (although, this patch cannot be merged back to 2.8, so if we want to backport it, we'll have to just do a strings change and leave the other code alone.)
So? Should I push it? For a simple string change on 2.8, yes I can do this of course.
Forgot about this, will review asap
The patch looks fine, please push. You are the expert in the new migration voodoo tho, I trust this does what it should.
It does what it should, and if not, that can be fixed. :-) Anyway I guess I should write some test for it. I'll put this on my TODO. commit 4b14ed2e5e0c5a83df361814c4c8286079f60989 Author: Jehan <jehan@girinstud.io> Date: Wed Nov 13 15:15:45 2013 +1300 file-export* labels and actions renamed. Follows updated save+export specification. For renamed actions (file-export and file-export-to respectively to file-export-as and file-export to mimick file-save*), menurc from GIMP 2.8 will be correctly migrated. I've also pushed a 2.8 commit with only the string changes, as proposed by Drawoc: commit 3097b94e614841150c6790c1932f85b51a261812 Author: Jehan <jehan@girinstud.io> Date: Mon Nov 18 09:44:10 2013 +1300 file-export* labels renamed. Follows updated save+export specification. Partial implementation of commit 4b14ed2. The full commit also renames the actions for consistency. But I realize just now this might not be appropriate since you said that there should be no string change in the stable branch. Was it a mistake? Should I cancel the 2.8 commit? Anyway if that was a mistake, just tell me, I'll revert the patch, I guess.