d2phap / ImageGlass

🏞 A lightweight, versatile image viewer
https://imageglass.org
Other
7.9k stars 502 forks source link

Unhandled Exception - "Save As" across network #1055

Closed fire-eggs closed 3 years ago

fire-eggs commented 3 years ago

This is a report of Allan Holdt's bug from Google Groups.

System information:

To Reproduce

Steps to reproduce the behavior:

  1. Double-click an image in network shared folder.
  2. Goto the File menu, select "Save As".

Actual behavior:

Unhandled exception

image

Expected behavior:

User is presented with the save dialog box.

Screenshots:

Additional context:

The network shared folder must have a "large" number of images. On the order of 15,000.

fire-eggs commented 3 years ago

I've finally reproduced this bug.

I was able to reproduce it by opening an image across the network, with "find in child folders" ON, and the image at the top of a tree of folders - 11,000 images. Admittedly, I have to go to "File/Save" or "File/Save As" fairly quickly.

The problem relates to the change added where the "current" image is displayed while IG builds the image list in the background. The image is visible, but operations which require the full list to be fully initialized will fail ... until the full list is fully loaded.

There is a window of time between when the user clicks on an image and the list is fully loaded. That window can be pretty long when there are a lot of images, and/or accessing across the network, especially a slow network.

Specifically, until the list is fully populated, attempting to access the "current" file will return null. E.g. as used in mnuMainSaveAs_Click:

var currentFile = Local.ImageList.GetFileName(Local.CurrentIndex);

During the mentioned window of time, Local.CurrentIndex is -1, and currentFile will be null.

My attempted fix via #1050 will not fix the crash. I addressed the Substring error, but did not realize the currentFile was null.

fire-eggs commented 3 years ago

A quick scan of the usages of Local.CurrentIndex suggests the following functions might be impacted. The functions may crash (failing to check for null filename) or do something the user doesn't expect (e.g. Copy Image Path will put an empty string in the clipboard, not the path of the apparently visible file).

mnuSaveImage_Click mnuMainSaveAs_Click (show ExifData in Local_OnImageChanged) picMain_MouseDown SelectCurrentThumbnail UpdateEditAppInfoForMenu RenameImage CopyMultiFiles CutMultiFilesAsync MnuMainNewWindow_Click mnuOpenWith_Click mnuMainEditImage_Click mnuMainPrint_Click mnuMainMoveToRecycleBin_Click mnuMainDeleteFromHardDisk_Click mnuMainExtractPages_Click mnuMainImageLocation_Click mnuMainImageProperties_Click mnuMainCopyImagePath_Click

fire-eggs commented 3 years ago

This issue is a pain to make happen [setting up a remote folder with 15000 images]. For testing and debugging purposes, the problem can be replicated by adding a forced delay in PrepareLoadingAsync. Either within the await Task.Run invocation, or within LoadImages, adding a sleep(20) would simulate the problem. Then, when loading a local image, there is a 20 second window of time where the usages of Local.CurrentIndex could be tested.

d2phap commented 3 years ago

Fixed in the latest build of ImageGlass Moon: https://imageglass.org/moon