Open image in external viewer

[Danielle Foré]

Could you add menu item (on right click) that opens image in default external image viewer. It is useful for me because I often look for EXIF and other data in images. Epiphany/Gecko used to have this option.

[Peter Hatina (phatina)]

I'd like to hear an opinion from Christian or Daniel, this is the first try.. It runs a default picture viewer associated with the file extension. Originally, I was thinking of having a dialog with some picture viewing apps, that could do the job... But gtk+ 2.x doesn't have this feature (introduced in gtk+ 3.0). To support this with gtk+ 2.x, we can use some existing code, which does this, but is it worth doing this?

[Cris Dywan (kalikiana)]

Please refactor the filename guessing and rename-if-file-exists from midori_view_download_requested_cb into a function, it is important to get the details right here.
Really use the temp folder, I would not want files to appear in my Downloads or Pictures folder unless I save them myself.
Possibly use a better token than save-to-tmp, say open-in-viewer. The same code could be used for video files I imagine.
Nitpick: Open with default image viewer → Open in Image Viewer, I think 'default' is redundant here.

[Cris Dywan (kalikiana)]

As for choosing an image viewer: I would say GTK+3 only if there is none set, otherwise nothing. It's not worth re-implementing the wheel here for something less needed if you ask me.

[Peter Hatina (phatina)]

I also added a logic, which downloads the image every time and saves it with a number in parenthesis right before a file extension (if such file exists).

[Cris Dywan (kalikiana)]

Looking very nice. midori_browser_download_status_cb still should be more robust. It needs to check if the download failed, otherwise it could be waiting forever. And it should show a UI error rather than g_error, which would be an unexpected crash in the perception of the user.

I like user-specific temporary folders here. Nice idea from a data privacy point of view and reducing noise in /tmp.

I suggest these follow-ups after getting the initial patch in, they can be separate bugs:
- Unit tests for midori_browser_download_prepare_filename
- GTK+ 3 app choice fallback
- Use new functions in existing download opening code

We may be having a release this week and I would wait with this feature so we don't get a new very visible string just before release.

[Peter Hatina (phatina)]

There are 2 uncertain places in the code - Christian, I'd like to hear your opinion about this.

1) Do we need to raise message dialog, when an error occured while downloading a picture?
2) What kind of separator would you use? Current code uses parenthesis, but pfor suggested underscore or dash - simple reason, they do not need to be escaped, when using cli. I used parenthesis, because it seems to me more convenient (more browsers use parenthesis).

Both questionable places are marked with TODO comments.

[Cris Dywan (kalikiana)]

gtk_message_dialog_new should take the browser as the parent window.

1) Yes, at least say "Download failed". Not sure if we can get the error message easily, but we don't want users waiting "why is nothing happening there".
2) I'm used to seeing parantheses, but I agree they can be inconvenient. I don't know if there's a convention in use with underscore or dash, I am fine with either.

[gue5t gue5t (gue5t)]

I'd think that if the reason to use a dash or underscore is ease of typing, dash would win out since it doesn't require pressing shift in common layouts. Otherwise I don't know of a significant convention for one over the other. Alternatively, the format used for filenames could be configurable; this would seem a decent compromise and assist if users want to ensure consistency between different programs that generate temporary files.

[Peter Hatina (phatina)]

ad 0, 1 - done
ad 2) I am also used to see parenthesis, but agree with gue5t, that dash can be the best separator - no escape needed and at most layouts, it does not need shift to be pressed

[Cris Dywan (kalikiana)]

I modified the patch to use sokoke_message_dialog and I added missing _() around the messages. I also committed a separate patch to use the new functions for regular downloads.

Small glitch: you can try to open stock:// images in error pages but they won't do anything. But not really something to worry about.