Opened 2 years ago
Last modified 2 years ago
#17803 new bug
ShowImage will show an old file after overwriting it if a window stays open
Reported by: | fatigatti | Owned by: | leavengood |
---|---|---|---|
Priority: | normal | Milestone: | Unscheduled |
Component: | Applications/ShowImage | Version: | R1/Development |
Keywords: | Cc: | ||
Blocked By: | Blocking: | ||
Platform: | All |
Description
To reproduce, you need (say) three image files, namely file1.png, file2.png and file3.png.
1) Open file1.png with ShowImage and leave that window open. 2) Open file2.png, see what's the content, close this window. 3) Delete or rename file2.png and then rename file3.png to file2.png. 4) Double click file2.png in Tracker. ShowImage will show the old file instead of the current one.
This doesn't happen if no ShowImage window stays open, so it's probably some kind of caching. Note that file1.png doesn't even need to be in the same folder (at first I tought it was caching the entire folder contents so it seemed "logical", if a little strange). As long as there's a ShowImage process running you can reproduce the bug.
How I run into this? I was trying to export a genealogy tree to png and I didn't frame it right the first time, so I simply overwrote the file and for some reason I had a ShowImage already opened. Definitely a common use case (i.e. you edit an image, don't get it right, overwrite, open to see the new one).
Change History (4)
comment:2 by , 2 years ago
It may be a good idea to node-monitor open images and pop up an alert:
The image "%filename%" appears to have changed. [Cancel] [Reload]
That way, you get notified as soon as the image changes, not just when double-clicking the file. Plus, you easier see the difference when concentrating on the image when clicking "Reload".
comment:3 by , 2 years ago
This is indeed happening on a very recent hrev (2 or 3 days old max). Good to see there's already some hint about what the problem might be.
@humdinger, I like your proposal and I think it would make a nice feature, but just to clarify things, this is happening even with the file closed (you probably understood that and in that case I didn't say anything). An open ShowImage process, on any picture on any folder is all it takes for this to happen. Not monitoring opened files for changes is not ideal, but I've definitely seen that before and can live with that. On the other hand, double clicking a file just to get another...
comment:4 by , 2 years ago
I'll test to reproduce but this is likely due to the caching as ttcoder deduced. If you have the actual file being replaced open then the node monitoring should update it. But I am fairly sure the caching code just loads the file and caches the bytes so that when you go to the next file with the arrow keys you don't have to wait for it to load. So if the file changes underneath it still has the old bytes. It also obviously uses the cache if you open a file that is already cached, which is this bug in question. But I think it also is a bug if you replaced file2.png and then moving to it in the slideshow showed the old file (and I assume that is indeed what happens now.)
I think the solution will be to have the caching code do node monitoring and reload on change. Though that might get tricky and I feel like this code may already have some data races.
I've been wanting to fix some issues in ShowImage for many years but haven't had Haiku dev time for a while but I should finally have a dev setup again soon.
Yea.. I think there might be a ticket for this already. (is that for a recent hrev ?)
I'm not certain which part of the code is to blame but I seem to remember it's here : https://git.haiku-os.org/haiku/tree/src/apps/showimage/ImageCache.h
It uses an entry_ref (one of the very few bad ideas that Be came up with in their API). Thus when you double-click the "new" file2.png, it goes "ok, this is the same device, same directory, and the filename matches, so I can pull the old Bitmap from the cache, it's clearly the same file".
Where it is not the same file obviously (not the same inode). But it doesn't know that.
In my own builds I disable that part of the caching code as I find that bug very annoying indeed.