Overwrite export

[madd-games]

This PR adds a menu entry which allows the user to overwrite the file at the stored export path.

I tried as much as possible to make it work like GIMP. It would appear that GIMP shows the path when saved, export_path only if there is no path, and "Untitled" otherwise. It also has an "(exported)" label at the end of the title bar if there is an export_path (even if there is also a path).

I used Ctrl+E for export.

One consideration, tho: should we store file_format_t *export_format in image_t alongside the export_path? I'm not certain how the save dialog works, but in goxel_export_to_file(), if the dialog were to return a path ending with, say .png, but the format was "txt", it would actually export a TXT file with .png extension. But then, hitting Ctrl+E would write it as PNG, since this time it detects the format based on the name. This is only if there is a possibility of the dialog returning the wrong extension tho.

file_format.h actually includes image.h, so if I were to implement this, I'd have to reference it as struct file_format *export_format;, unless you have some other suggestion.

In any case, let me know.

NOTE: I do not actually speak French so please verify the translation if possible. Thanks.

[guillaumechereau]

Thanks. it looks good. A few changes are needed (I assume you can git push -f into your branch? I never really used github PR):

• Your commit names are different from the other ones (Mariusz Pilipczuk instead of Madd Games). If you want to use both you have to add it to the CLA as well.

• The menu item to overwrite should only be present if there is an export path (no need to show it otherwise, even disabled).

• The menu item should state the name of the export path (see gimp). To do that you need to snprintf into a local buffer, something like: snprintf(buf, sizeof(buf), "%s %s", _(OVERWRITE), export_path).

• this code:

char *new_export_path = strdup(path);                                                                                                                    
free(goxel.image->export_path);                                                                                                                          
goxel.image->export_path = new_export_path; 

can be simplified to:

free(goxel.image->export_path);
goxel.image->export_path = strdup(path)

• Finally, the codding style I prefer is to put local variable declarations on top of the functions, and limiting line width bellow 80 chars. I explain the style in more detail in 'CONTRIBUTING.md'.

I think it would indeed make sense to store the export format in addition to the path like you suggested. Maybe you can look at this after we merge this pull request.

[madd-games]

Hello,

• My username is madd-games and the files is called madd-games.md, but my legal name is Mariusz Pilipczuk which is what I added to the declaration. I pushed with the same settings on both branches. Is another declaration still needed? If so, tell me the file name and which name I should use (legal or madd-games).

• Alright, I can do that.

• Same here, will do that.

• This code cannot be simplified, because like the comment states, a_overwrite_export() calls goxel_export_to_file(goxel.image->export_path, NULL), so path is the same pointer as goxel.image->export_path. Thus, if I free(goxel.image->export_path);, that's the same as free(path), and we're immediately calling strdup(path) afterwards, thus using a freed pointer. I actually tried the simple way initially and encountered the memory issue.

• I'll see, it should be easy enough to integrate this into my current branch, so no need for new PR. I'll get back on that.

As for continuing the pull request, all I have to do is push changes into my local branch and they will show up here, so no worries.

I'll implement the changes and improvements later today.

[guillaumechereau]

On Wed, Mar 20, 2024 at 4:42 PM Madd Games ***@***.***> wrote: Hello, My username is madd-games and the files is called madd-games.md, but my legal name is Mariusz Pilipczuk which is what I added to the declaration. I pushed with the same settings on both branches. Is another declaration still needed? If so, tell me the file name and which name I should use (legal or madd-games).

Ah I see. Well should be fine then. I was just a bit confused in the commit logs to see two names.

This code cannot be simplified, because like the comment states, a_overwrite_export() calls goxel_export_to_file(goxel.image->export_path, NULL), so path is the same pointer as goxel.image->export_path. Thus, if I free(goxel.image->export_path);, that's the same as free(path), and we're immediately calling strdup(path) afterwards, thus using a freed pointer. I actually tried the simple way initially and encountered the memory issue.

OK I see! Why can they be the same though? Need to check that and simplify if possible, cause here it's quite confusing otherwise. Do you think you can fix that? If not ok, leave it like you did. Thanks again for the work. Cheers.

[madd-games]

It's equal because we want to use the already existing export_path when using 'overwrite export'. The only other solution I can think of would be to call strdup() and free() inside a_overwrite_export(); but that might be more confusing than the current approach.

I do agree that it's a little awkward to have to use the new_export_path trick, but I just don't have another idea for now.

[guillaumechereau]

Right, I didn't read the code properly. Tricky indeed.

…

On Wed, Mar 20, 2024 at 5:09 PM Madd Games ***@***.***> wrote: It's equal because we want to use the already existing export_path when using 'overwrite export'. The only other solution I can think of would be to call strdup() and free() inside a_overwrite_export(); but that might be more confusing than the current approach. I do agree that it's a little awkward to have it there, but I just don't have another idea for now. — Reply to this email directly, view it on GitHub <#362 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAA2JH4FA6ZQXYFK25Q3ABDYZFG4BAVCNFSM6AAAAABE6CDJLSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMBZGA4DGMBSHE> . You are receiving this because you commented.Message ID: ***@***.***>

[guillaumechereau]

Let me know when I can merge then, I think this is good now.

[madd-games]

I think I fixed all the problems. I've changed file_format_for_path() so that if name is given, it only returns that format (currently, if the path indicates a different format, and that format happens to come first, it'll ignore name; my changes enforce the use of name). I also renamed the function to get_file_format() since it doesn't necessarily work by just the path, but let me know if you disagree.

I included the %s inside the translation string for the menu item itself. This allows us to have languages where the file name should not be at the end. But let me know if you dsiagree and have an alternative solution.

Thanks

[guillaumechereau]

Should be fine. I merged the branch, thanks!