ENH: report errors, retry in saving camera image #21

ZLLentz · 2024-11-27T02:27:28Z

Related jira: https://jira.slac.stanford.edu/browse/ECS-6918

This PR's primary purpose is to fix a bug where the image saving dialog always reports success.

The reason for this is that QImage.save never raises, instead it returns True or False to signal if the image was saved properly. The original author of this bit of code assumed that the save function would raise with information about what went wrong.

This ends up being somewhat annoying, leaving it to us to figure out why the save might have failed.

Changes implemented here:

Correctly check if QImage.save succeeded or failed
If it failed, do a best-effort check for simple issues
If it failed, ask the user to try a different filename and re-open the dialog
Set the default directory to the user's home area, which is guaranteed to be writeable, unlike the current working directory
fileName -> filename because the naming was irritating me

…e writeable

KaushikMalapati · 2024-11-27T07:18:09Z

camviewer_ui_impl.py

@@ -17,6 +17,7 @@

 import sys
 import os
+import os.path


Why did you import os.path if you already had os?

I was under the mistaken impression that import os did not expose the utilities in os.path, because this is how all (most) other modules work. I'll remove the redundant import. I suspect this is a habit I picked up by reading code from other programmers.

There's some interesting implementation details about this I was unaware of, namely that the os module imports either posixpath or ntpath based on your OS and re-exports it as os.path, injecting it into sys.modules along the way. Pretty wild behavior, but I guess this is a reasonable way to aim for platform-independent libraries.

The os.path platform specifics is a TIL for me as well

I'm pretty sure I'd have just tried to access the path as a submodule of os, but Zach is right that this is generally not suggested. This access method only works if the parent module imports its submodules. This isn't guaranteed to work for all modules nor is it guaranteed to not change.

camviewer_ui_impl.py

tangkong · 2024-12-02T21:41:50Z

camviewer_ui_impl.py

+                # No write permissions?
+                else:
+                    # This is the most robust way to check if we have permissions
+                    # os.access exists but doesn't quite work fully on NFS filesystems as per the Python docs


I've seen os.stat mentioned in conjunction with the stat module, if there's a need for an alternate way? (stackoverflow)

I think os.stat also won't fully encompass the NFS permissions but maybe the accuracy drop will be worth the simpler implementation (not accidentally creating a stub file)

Resolved via simplifying with os.access to reduce compexity

Interestingly python suggests the ask-for-forgiveness (try-catch) approach for security reasons we probably don't care about.

Out of curiosity, do you remember what the NFS limitations were?

I don't remember the details, but in network file systems there can be additional file protections that go beyond the posix uid/guid stuff. I think in practice this isn't used for us.

camviewer_ui_impl.py

KaushikMalapati · 2024-12-02T23:49:12Z

camviewer_ui_impl.py

                "Images (*.npy *.jpg *.png *.bmp *.pgm *.tif)",
            )[0]
-            if fileName == "":
+            if filename == "":


Is it intentional to exit if there's no filename provided but retry if there was a saving problem?

It's not documented very well (https://doc.qt.io/qt-5/qfiledialog.html) but empty string is returned if the user clicks "cancel".

Is this meant for this comment #21 (comment)?

No, I meant to respond here in this way, but maybe I didn't justify myself fully.

The intention is to exit if the user clicks "cancel", which is signaled by there being no filename. Otherwise the user has no way to change their mind and not save anything.

Feels slightly wrong to throw an exception and have a warning popup if you press cancel, but not sure if it's worth making more user-friendly

Ah yeah, you're right- there's a UX problem here. I forgot what came next when responding to this comment since it was pre-existing code. I'll clean this up.

Resolved by making this code path give no message at all. No raise, no pop-up- the UI element that gets us to this point doesn't let you input an empty string, so the only way to be here is to click "X" or "Cancel", so you don't need this confirmation pop-up in any form. Added a comment to clarify intent.

KaushikMalapati · 2024-12-03T00:14:13Z

camviewer_ui_impl.py

-            if fileName.lower().endswith(".npy"):
-                np.save(fileName, self.image)
+            if filename.lower().endswith(".npy"):
+                np.save(filename, self.image)


Just double checking, but can this fail silently? In the cases you check for if QImage.save fails (no write permission, file already exists, other), can this fail by throwing an exception which would not result in a retry? Also looking at https://github.com/numpy/numpy/blob/v2.1.0/numpy/lib/_npyio_impl.py#L507-L588
I think that this could override an existing file with the same name which shouldn't be possible in case someone tries to use the same filename for different images and ends up overwriting all but the last one.

You're right that I need to be more careful here. The exception being thrown would be "safe" but wouldn't result in a retry, giving inconsistent behavior.

I'll double-check about re-using the same name, but my fuzzy memory is that the file dialog gives you an "are you sure?" sort of error, so if we reach np.save the user has already confirmed that they want to replace the file.

Resolved by refactoring to make specifically OSError call the same method that we reach via the QImage.save checks.
Confirmed that the file select dialog warns the user sufficiently if they pick an already existing filename.

KaushikMalapati · 2024-12-03T00:18:58Z

camviewer_ui_impl.py

@@ -1151,30 +1151,58 @@ def shutdown(self):

    def onfileSave(self):
        try:
-            fileName = QFileDialog.getSaveFileName(
+            filename = QFileDialog.getSaveFileName(


If you clicked on the "Save to File" button and then decided not to save anything by exiting the dialog, what happens here? Does getSaveFileName return nothing causing an exception?

It ends up returning an empty string. Qt APIs can be awkward like that.

Resolved by removing the exception as stated above

KaushikMalapati · 2024-12-03T00:29:50Z

camviewer_ui_impl.py

+                # QImage.save returned False, so the save failed.
+                # Check for obvious errors, then retry the save
+                # File already exists? This shouldn't happen due to the qt file dialog.
+                if os.path.exists(filename):


Can QImage make the fail but still return False?

The API docs claims that it cannot, but it's good to be skeptical, especially with pyqt as a middleman between us and the qt docs. Maybe I should also simplify here and be less ambitious in checking some of these things.

https://doc.qt.io/qt-5/qimage.html#save

Resolved by abandoning this check in favor of large simplification. This at least avoids an incorrect message in the case where QImage would have undefined behavior, and it allows the user to freely override existing filenames if they want to for whatever reason.

ZLLentz · 2024-12-03T03:49:14Z

In addition to the above, I added a check for invalid file types since it's the only other situation I could find where QImage.save would fail aside from permissions issues. There's a limited, predefined set of image types that are valid.

ZLLentz · 2024-12-03T03:50:12Z

The diff is somewhat confusing now because it's more like a rewrite than a tweak, but I think this is cleaner than before!

camviewer_ui_impl.py

ZLLentz added 3 commits November 26, 2024 17:37

ENH: give onFileSave some failure reporting

7da3341

DOC/ENH: expand comment, add a fallback reason

2c5eb9b

ENH: switch file save default dir to home area to make it likely to b…

ee3ace1

…e writeable

KaushikMalapati reviewed Nov 27, 2024

View reviewed changes

MNT: remove redundant import

1ba0db8

ZLLentz marked this pull request as ready for review December 2, 2024 19:22

ZLLentz requested review from tangkong, KaushikMalapati and janeliu-slac December 2, 2024 19:23

tangkong reviewed Dec 2, 2024

View reviewed changes

KaushikMalapati reviewed Dec 3, 2024

View reviewed changes

ZLLentz added 2 commits December 2, 2024 18:32

ENH: retry image save using qt signal instead of recursion

7f25139

ENH/FIX: Simplification, consistency, clarity, warn on invalid file type

44794f6

tangkong reviewed Dec 3, 2024

View reviewed changes

camviewer_ui_impl.py Outdated Show resolved Hide resolved

ZLLentz added 2 commits December 3, 2024 09:13

FIX: leading . for clarity and to match ext var

87a3dc4

FIX: handle case where file exists and we can't write to it

4205c76

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: report errors, retry in saving camera image #21

ENH: report errors, retry in saving camera image #21

ZLLentz commented Nov 27, 2024 •

edited

Loading

KaushikMalapati Nov 27, 2024

ZLLentz Dec 2, 2024

tangkong Dec 2, 2024 •

edited

Loading

tangkong Dec 2, 2024

ZLLentz Dec 2, 2024

ZLLentz Dec 3, 2024

tangkong Dec 3, 2024

ZLLentz Dec 3, 2024

KaushikMalapati Dec 2, 2024

ZLLentz Dec 3, 2024

KaushikMalapati Dec 3, 2024

ZLLentz Dec 3, 2024

KaushikMalapati Dec 3, 2024

ZLLentz Dec 3, 2024

ZLLentz Dec 3, 2024

KaushikMalapati Dec 3, 2024

ZLLentz Dec 3, 2024

ZLLentz Dec 3, 2024

KaushikMalapati Dec 3, 2024

ZLLentz Dec 3, 2024

ZLLentz Dec 3, 2024

KaushikMalapati Dec 3, 2024

ZLLentz Dec 3, 2024

ZLLentz Dec 3, 2024

ZLLentz commented Dec 3, 2024

ZLLentz commented Dec 3, 2024

ENH: report errors, retry in saving camera image #21

Are you sure you want to change the base?

ENH: report errors, retry in saving camera image #21

Conversation

ZLLentz commented Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tangkong Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZLLentz commented Dec 3, 2024

ZLLentz commented Dec 3, 2024

ZLLentz commented Nov 27, 2024 •

edited

Loading

tangkong Dec 2, 2024 •

edited

Loading