feat(iv): flip, rotate and save image

[vangeliq]

Fixes #4715

Description

• Added save functionality
• Added flipping and rotation capabilities via metadata.

Note that the actual pixel data remains unchanged, so only image formats that support the rotation metadata will reflect the changes.

I thought that it made sense for the flip and rotate to be under the tool section, similar to where the macbook Preview app places it. Keyboard shortcuts also follow the shortcuts that Preview uses.

Tests

Used a .jpeg photo to test:

• saving
• making changes with/without saving and re-opening the image
• flipping and rotation from different initial rotation/mirrored states

Otherwise there doesn't seem to be a test suite for iv?

Checklist:

• I have read the guidelines on contributions and code review procedures.
• I have updated the documentation if my PR adds features or changes
  behavior.
• I am sure that this PR's changes are tested somewhere in the
  testsuite.
• I have run and passed the testsuite in CI before submitting the
  PR, by pushing the changes to my fork and seeing that the automated CI
  passed there. (Exceptions: If most tests pass and you can't figure out why
  the remaining ones fail, it's ok to submit the PR and ask for help. Or if
  any failures seem entirely unrelated to your change; sometimes things break
  on the GitHub runners.)
• My code follows the prevailing code style of this project and I
  fixed any problems reported by the clang-format CI test.
• If I added or modified a public C++ API call, I have also amended the
  corresponding Python bindings. If altering ImageBufAlgo functions, I also
  exposed the new functionality as oiiotool options.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: vangeliq / name: Valery Angelique (ee943e6)

[vangeliq]

Added my new email for the CLA check. Rebased and force pushed with --signoff as per the DCO message.

git rebase HEAD~4 --signoff
git push --force-with-lease origin feat(iv)-flip,-rotate-and-save-image

[lgritz]

Still having DCO problems. Can you rebase on current main and squash, adding your sign-off?

I believe that can be done with the following commands (assuming your branch is checked out):

Rebase on current main:

git rebase main

Squash and add DCO sign-off:

first_commit=$(git rev-list --reverse --ancestry-path main..HEAD | head -n1) && git reset --soft main && git commit -s --reuse-message=$first_commit

[lgritz]

Flip H and V seem to work great.

I think your rotate left and right are swapped.

Should these be under "Tools", or "View"? I keep looking instinctively under View. But when I look at the Apple preview utility, the equivalents are under Tools. So maybe this is right.

I'm unsure about cmd/ctrl-S being an unconditional save. Seems easy to hit accidentally and overwrite your original input. Should I be worried about that?

Does anybody else have any opinions about my last two questions?

[lgritz]

We're trying to replace stream output with print:

Suggested change

	std::cerr << "Save failed: " << img->geterror() << "\n"
	<< "Try using `Save As` instead."
	<< "\n";
	OIIO::print(stderr, "Save failed: {}\nTry using `Save As` instead.\n",
	img->geterror());

[vangeliq]

There's a couple references to stream output on the file, should I replace it with print as well?

Or maybe just the reference on the save as function since that's somewhat related to the issue.

[lgritz]

We're trying to shift from stream (<< nonsense) to C++23 style print() style. We use the fmt library underneath, so we don't need to wait for C++23 to be our minimum. We definitely don't want to add any new stream output in new code, but it's not required that you go fix all stream output in a file you're in, unless you happen to be changing those very functions anyway.

As an aside, it's really not a good idea to have any console output at all from a GUI app like iv. You can't count on anybody hhaving the console window (from which the app was launched) being visible, or even existing. I understand it's even more problematic on Windows for a GUI app. Probably errors like the inability to write to a file should be handled with a dialog box. But you don't need to do that as part of this PR. Maybe we should file a new issue (perhaps providing somebody with a good DevDays project in the future) to root out any console output in iv and replace it with dialog pop-ups.

[vangeliq]

Ignoring the fact that I spent half an hour trying to understand why I wasn't able to get the latest commits from main (spoiler alert, I forgot I was on a fork and had to fetch from upstream), the git commands you gave me worked amazingly.

I think you're actually a git wizard?

[lgritz]

Ignoring the fact that I spent half an hour trying to understand why I wasn't able to get the latest commits from main (spoiler alert, I forgot I was on a fork and had to fetch from upstream), the git commands you gave me worked amazingly.

I think you're actually a git wizard?

I know how to do the things I tend to do every day, and vast other areas I have to look up every time.

I admit that I usually do such things with git rebase -i and just doctor each commit by hand to 'f(ixup)'. I didn't want to have to explain that to you if you hadn't used rebase -i before or didn't understand what it was doing, but I did not know the command that would do it all in one step. I asked ChatGPT, and it gave me a clearly wrong answer (I did test on my end before relaying to you), but the suggestion got me close enough to know how to recognize the right answer on Stack Overflow.

[lgritz]

Minor problem here -- we were already using Ctrl+R for "reload image".

[lgritz]

The code LGTM and functionality seems correct in my testing!

However, looking at the hotkeys, I see that you assigned Ctrl+R to be "rotate right", which is totally logical except for the fact that we already used Ctrl+R for "reload image".

Maybe if we were starting fresh today, Ctrl+R/L would be better for left/right (and would match MacOS's Preview.app) and choose a different hotkey for "reload". But it's been this way for years and perhaps many users have muscle memory for Ctrl+R and will be confused if its meaning changes.

Maybe Ctrl+Shift+R/L can be used as hotkeys for rotate right and left?

Anybody else have suggestions for how to resolve this? Are there any other commonly used apps with an established convention that people might already be expecting?

[lgritz]

Hi, @vangeliq, I think the only thing we are waiting to resolve here is that you ended up hijacking a hotkey that was already in use.

You know, I just thought of one more thing. I'm a little wary of just how easy it is to mistakenly hit Ctrl-S and overwrite the file without any dialog or confirmation. Without saving files, nothing you can do in iv can cause any real damage since all you're doing is visualizing image files without altering them, so saving is the one thing we need to be extra careful about.

In addition to being possible to overwrite a file that you didn't really mean to change permanently, even when you have not changed the image at all, saving when you don't need to can cause potential harm in at least two other ways: (a) it will disrupt the file's time stamp, and (b) if the file uses a lossy compression (like JPEG or exr DWAA compression), it could slightly change/degrade the pixel data in the files with every needless read/save cycle.

I think that a large amount of the downside can be mitigated with two changes: (1) keep track of whether the user has done anything to change the image, and have the "save" operation do nothing (no writing to disk or altering the file) in cases where there has been no intentional change; (2) a dialog asking for confirmation that the user wants to overwrite the file.

I think also that this might be why we originally had only "Save As..." and not "Save" -- it forced a confirmation dialog, causing the user to think about whether they really wanted to save, and if so, what filename to use. Maybe just restoring that original state (no Save, only Save As) is also an expedient solution, i.e., without a Ctrl-S just saving immediately with no confirmation, there is much less chance for an accident caused by a slip of the finger or absentminded choice.

[vangeliq]

Hey @lgritz,

Thanks for the feedback! I'll update the hotkey bindings when I get back home tonight. I think your concern about accidental ctrl-s makes sense, I wasn't sure that it saved when I was trying test it lol.

I didn't consider how that would affect lossy compression files, so that's actually super cool to know and thanks for bringing that up.

I also tried to google what other hotkeys rotating uses, and the results I got were kinda all over the place, so I think I'll just remove them entirely as well. I guess R is just too popular of a letter for shortcuts 😅. Here were some that I found. if you want me to change it to any of these, then let me know. if not I'll keep it to no shortcuts.

• Ctrl + [ and Ctrl + ]
• R and shift+R
• ctrl+, and ctrl + .

TLDR on changes I'll make:

• remove Save button
• remove rotate shortcuts

[lgritz]

IF we hadn't already used Ctrl-R (Cmd-R on MacOS) for something, then Ctrl-R / Ctrl-L would be a good choice for rotating right and left, and it would match what Apple Preview uses.

But, alas, Ctrl-R is bound to an important and commonly used operation: Reload the current image -- which is super helpful if you're viewing an image which is getting periodically updated on disk.

Maybe I could suggest Shift-Ctrl-R / Shift-Ctrl-L (or on Mac, it would be Shift-Cmd-) instead? Still fairly mnemonic, but non-interfering?

[lgritz]

I think Ctrl+S was fine here all along -- because "save as" pulls up a dialog, giving the user a chance to pick a name or cancel the operation. An accidental hotkey invocation can't just unilaterally overwrite a file before somebody knows what they've done.

Ctrl+Shift+S would be fine for save-as if there was also a Ctrl-S for save. But it strikes me as an unnecessarily complex hotkey combination when there is nothing that needs to be disambiguated.

So my inclination is to just leave this particular line as it always was -- Ctrl+S is our saving operation, but it always means "save as" with a dialog. There is no unconditional "Save" to the current name without asking in iv, because it's just too dangerous.

[lgritz]

I think we are just that one comment I made away from being done with this PR. Thanks for your patience.

[lgritz]

Looks great, @vangeliq!

[lgritz]

Fixes #4715

[vangeliq]

Yay! Thanks for taking all the time to review my PR :D