Repository: karlch/vimiv-qt (language: python)

Issue:
Lazy Loading Overhaul
Included in this pull request are a number of changes to the way images are loaded by vimiv, with special focus on lazy loading. The code is most certainly not ready to be merged with the upstream project. Tests need to be written for it, the code generally needs to be cleaned up and the style made consistent, and any ideas that don't work well with the vision for upstream vimiv must be removed.

The rationale for these changes is primarily to accommodate the author's niche and specific use case. That is browsing sets of thousands of images, while minimizing memory usage and redundant disk IO. These images are typically sorted externally and fed in through stdin, so their order must be maintained.

The intention was to make all changes opt-in by means of configuration variables, the default values of which would maintain the original behavior of the software. These changes are already seeing use in my personal fork, so I will not be offended if they are turned down.

The following configuration variables are added to change the behavior of the software:

`thumbnail.save`        : If set to False, thumbnails will not be cached to the drive.

`thumbnail.max_ahead`   : Only this number of thumbnails after the current selection will be loaded. Changing the selection will modify the range of loaded thumbnails accordingly. If zero, load unlimited, as is the default.

`thumbnail.max_behind`  : Like `thumbnail.max_ahead`, but for thumbnails preceding the current selection.

`thumbnail.max_count`   : Thumbnails will not be unloaded unless the number of them exceeds this. Note, that this does not cause thumbnails to be forcibly unloaded, so this variable should probably be renamed. Anyway, set to 0 to immediately unload out of range thumbnails.

`image.id_by_extension` : If set to True, use the extension to assume the filetype, instead of reading with `imghdr`. This prevents large sets of images from being all opened when starting.

`image.imghdr_fallback` : If set to True and an image fails to load by a reader guessed by `image.id_by_extension`, try using imghdr to find an appropriate reader. In light of the pending replacement of imghdr, this should perhaps be renamed.

`sort.image_order` and `sort.directory_order` also now have a `passthrough` sort option, which will maintain the order that images were provided to vimiv from the command line or stdin. Will probably remove `sort.directory_order`, since it's unlikely to be very useful. And I don't think it works properly.


Discussion / hints:
Wow, thanks a bunch for the PR and all the work that went into this!

Would you mind splitting this into four distinct PRs for discussion purposes, as I do see parts being merged quickly, others needing more thoughts / discussion / time?

Some initial thoughts:

1. `thumbnail.save`: this would create quite some computational overhead in each step, no? Ages ago in the old version we removed this option with the implementation of the freedesktop.org thumbnail management standard (https://github.com/karlch/vimiv/issues/14). The computational overhead is worth avoiding disk writes for you? I suppose the path also changes quite often?
2. `thumbnail.max_behind`, `thumbnail.max_ahead` and `thumbnail.max_count`: funky, that must have been some fiddling :smile: Selling point is memory usage? If so, I like the idea, but agree with `max_count` being confusing. If the only goal is memory usage, I don't see why `max_count` is needed, if in addition it is CPU strain / disk reads, then I need more thinking, but lean towards removing that part.
3. `passthrough` sorting setting: can definitely be added, but currently unsure what the added value compared to a simple `"none": lambda x: 0` ordering function is. Only use-case I see is keeping the CLI order, like you mention. The others are more or less random for me. If you have additional use-cases, would love to hear them :blush:
4. Loading by extension instead of magic-bytes: as you mention, definitely makes sense to wait for #650 and then build upon this. I do see value in this as a setting, as the disk-reading strain has come up before in #363, and would love to merge a subsequent clean implementation. Feel free to just open an issue for this one, as the code would unfortunately have to be mostly re-written or at least moved after #650 :disappointed: 
Sure thing! And, I don't mind at all. The whole point of this draft pull request was to gauge sentiments towards these changes and figure out how to structure them.

1. My reasoning is a partly that espoused in #14. To avoid SSD writes, and also a preference for avoiding an endlessly growing thumbnail cache. Using freedesktop.org's thumbnail standard was definitely the right idea, in my opinion. However, the situation described where other software inevitably generates thumbnails is not the case for me, probably because I rarely use graphical file managers.

   My answer to the computational overhead question is to use the `max_behind`, `max_ahead` and `max_count` settings being used to only load images that the user is presently concerning themself with, rather than loading every image listed. These should be able to be tuned such that the next images are loaded by the time the user manages to scroll to them.

2. The idea behind `max_count` as it is tentatively named, is to reduce overhead in the situation of the user jumping to an arbitrary position in the thumbnail list, then jumping back to another. Both actions might put the view out of range of `max_ahead` and `max_behind`, causing the files to loaded and unloaded in quick succession. `max_count` allows for some out of range images to remain in memory. So, the goal is to allow some flexibility in memory conservation vs CPU usage.

3. Indeed, the `passthrough` setting is pretty much exclusively to preserve command line order. I'm sorry, I'm a little confused what you mean by the others seeming random to you.

   The approach of using `lambda x: 0` is better than a special condition to skip the sorting process, but in order for it to properly work in the even that the sort order is changed, a record of the command line order must be maintained.

4. Got it. I'll hold off until #650 is rolled out, and adapt my changes to that.
Thanks for your explanations!

1. Makes sense, happy to merge the option then, but should definitely be `false` at default (as you nicely did). In any case I would still keep the `max_*` settings in a separate PR, as those probably involve more review and discussion.
2. Understood, and sounds good to me keeping all of them. This one is probably the most involved to test and review.
3. "Others random" only refers to what happens when I e.g. move to a new directory in the library, or look at the library at all in this case, `os.listdir` and friends throw files in an order which just seems random to me, no clue how it works under the hood.
Ahh, now I see why this was added. I personally don't think this additional boilerplate is worth it though. If one wants the startup "unsorted", just set the setting upon startup. I would assume you always have this one set to `passthrough` or `none` by default?
4. Perfect :+1: 
Sure thing! I have nothing to add concerning points 1, 2, and 4.

For number 3 yeah, I see what you mean now. According to the Python docs, `os.listdir` is an unspecified order, and I'd speculate it's probably dependent on whatever interface the operating system provides to list a directory.

And yeah, I would have passthrough/none set by default. Arguably, none/passthrough should just naively take whatever existing order, so it'd make sense for changing the sort mode to wipe out the original command line order. With the present behavior, it could be called `commandline` rather than `none` or `passthrough`. Part of me thinks it'd be more consistent and flexible to have a sorting option that restored the original order, but its use would indeed be fairly incidental and its implementation pretty messy. I can totally understand a decision not to incorporate it.

A tangentially-relevant question to the matter of thumbnail loading: Do you know if there are any memory leaks associated with creating QIcon, QPixmap, and QListWidgetItem? When scrolling to the end and then to the beginning again with the max_ahead and max_behind settings, it seems that at least for me, the memory usage will slowly climb upwards as QIcons are created and assigned to ThumbnailItems. It probably isn't too much of an issue, but it's frustrating to see that something is being allocated and not freed, and not being certain of what that is, and I've tried various methods to 'ensure' objects are deallocated, but to no avail.
While I agree that the option to reset the sorting would be more consistent, I would definitely argue that the added complexity is not worth it. Yet another option could be calling it `unchanged` indicating we don't touch whatever feeds in - or exists already.

I don't know of any, but am certainly not an expert on the matter. Would be a bit surprising to me though if this is an upstream issue. I can reproduce this, but playing around with the memory usage on `master` just jumping around a few folders I found the behaviour to be quite unpredictable (does increase / decrease when moving to folders with more / less images, but nowhere near the amount I would have expected, sometimes goes in the opposite direction, havoc). Not sure how simple of an issue this is :thinking: 
It really is an edge case with a relatively complex implementation that goes against how Vimiv generally handles arguments. So, I could also imagine it potentially being a pain to work around in the future. Let's just do the sorting option without keeping a record of the initial order, then?

Part of me just wants to keep the name of the sorting mode simple with 'none'. It seems like the first thing that'd come to mind with most people? But, I will defer to you, if you have a preference there.

Concerning the 'memory leak', I honestly was imagining this might be something from the C++ side of QT, or perhaps it's a quirk in how Python's GC works. It doesn't *seem* like something in vimiv itself, though I could be wrong on all those counts. This may be one of those behaviors that is technically a feature, that there's some reasoning for this extra memory being allocated. Anyway, I think it's a little quirk to keep an eye on.

Finally is the name of `thumbnail.max_count`. Perhaps it should be something like... `thumbnail.unload_threshold`

And otherwise, this should be four separate pull requests for

* `thumbnail.save`

* `thumbnail.max_ahead`, `thumbnail.max_behind`, `thumbnail.max_count`

* `image.id_by_extension`, `image.imghdr_fallback`

* `sort.image_order`, `sort.directory_order`

If this all looks good, I think I have what I need to move forward and will close this draft. Thank you for considering my ideas and taking the time to talk them out with me.
Perfect, sounds good to me :+1: I am also totally fine with naming it `none` then.

Definitely worth keeping an eye on the memory issue, especially if this is an area where you have expertise. Personally, I probably won't be able to spend much time and extra thoughts on it given the general time limitations. Happy to help if anything specific comes up though of course.

The four PRs sound good, although the `image.id_by_extension` part may be better suited as an issue waiting for the mentioned PR to be merged. So three for now.

Concerning the naming of `thumbnail.*`, I think it would be nice if they had some common base that clarified them. We can discuss this in detail in the specific PR, but something along `load_before`, `load_after` and `load_max` maybe? While nit-picking, I think 0 should mean 0, and -1 = no limit (default). Was going to defer this to the PR, but while we are at it :blush: 

Thanks for your ideas and taking your time to share and discuss them!
I am a bit late to the party (did not have time yesterday to answer), but I also want to add a few comments 😊

2. I get your issue with the thumbnail. But some things are not yet completely clear to me, respectively I have the feeling that there is potentially a better way to implement that.

- Why exactly is there a distinction between `max_behinde` and `max_before`? Is that really needed?

- If I get you correctly, the intention off `max_count` is that the thumbnails generated while jumping through the library and finding the correct set of images, get eventually removed again (once enough *correct* images were loaded). Does it also delete already generated thumbnails from disk?

- Do you intent to change these option during runtime. E.g. once you identified the right location, increase `max_before/after`?

- I find the `max_count` option still a bit weird, as it does not consider the number of thumbnails generated and saved by vimiv in total, or thumbnails generated in different directories.

I have also an issue with the way thumbnails are generated. While my problem is different from yours, my solution may also (partially) solve your issue (or a combination of both solutions). I deal with many RAW images. As the extraction of the embedded thumbnail needs quite some time, and as vimiv generates previews starting from the top, it takes an eternity (up to several minutes) till the thumbnails of the last images, which are the ones I am concerned with (when dealing with an SD card, where the newest images are by default at the bottom), are loaded.

While your solution may (partially) solve the my issue, I still want that in the end, all thumbnails are generated.

To solve my issue, I thought that it would be nice if priority is put onto the thumbnails displayed in the current view. As soon as the view changes, the new set of images is prioritized. Once all images in the view are generated, the vimiv deals with the generation of all other, not yet generated, thumbnails (in a somehow "smart" (load what is probably used next) way).
This behaviour would be inline how it is done in other major image viewer/editing software.

How this addresses your issue. If we add a boolean setting that specifies that only images in the current view should be loaded, it would kind of have the effect you wanted to achieve with `max_before/after`. It may remove some flexibility (i.e. being able how many images around the selected one should be loaded), but I think it makes the intend of this option much cleaner and applicable to a wider user-base.

The `max_count` option can also be added to this solution.

Would that also solve your issue?

4. I have also thought about an option to let vimiv exclusively consider the file extension; If I have a SD card with a few thousand images of a single type, such an elaborated magic byte check is really not needed.
You will be amazed how easy this will be to add to the new implementation. This option comes pretty much for free 😊
Alright, I'll try to respond to everything roughly in order.

@karlch

The sort type will be named `none`, then!

I don't know if my expertise is QT5's memory allocation in particular, but I'll most certainly keep it in the back of my head, and let you know if I come up with any tangible leads on it.

Definitely still waiting for the `imghdr` replacement for `image.id_by_extension`

The changes to `thumbnail.*` make sense, and I only have one issue. That is `load_max` might not precisely describe what the option does, because thumbnails that exceed this number are not automatically unloaded if they fall into the range specified by `load_before` and `load_ahead`. Rather, images that fall outside of the range will not be unloaded, unless there are more images than that number. That's why I considered `unload_threshold`.

Anyway! Now to @jcjgraf

Sorry, I might be missing something. I don't think there will be two options to that effect. Originally, they were `max_ahead`, `max_behind`, and `max_count`, though they will be renamed to `load_ahead`, `load_behind`, and potentially `load_max`

Already generated thumbnails will *not* be deleted from the disk by any of this code. Only potentially unloaded from memory, so they'd need to be reopened, and that can be turned off. One option adds the ability to not write new thumbnails from the disk, so the original will be reopened instead.

For specifics on `max_count`, maybe the end of my response to karlch would help?

Changing the options during runtime is definitely something I should test, though tentatively, I think they should *probably* work.

That's a great idea about generating thumbnail files in the background after the current range is displayed, and I'm totally happy to implement it.

Finally, using a single option would be cleaner as you mention. My preference for specifying a number ahead and behind is that it can be tuned to slower hardware, where one would want to preemptively load more thumbnails. Could make `max_ahead` and `max_behind` into a single option that uses the same number for either, though.

So to be clear, it should go like this?

Generate and display thumbnails in the range specified by `max_ahead` and `max_behind` (or our hypothetical single variable), starting with those that are actually visible. For images outside the range, save the thumbnail files to the disk, if that is enabled (which it is by default).

Definitely happy about the file extension detection. The trickier part of it is trying to use the tentatively named `image.imghdr_fallback`, where if an image type auto-detected by the extension does not work, then it will try to read the header of the file. Currently, my implementation of this seems to work, but is currently *very* hacky. This is something that might become clearer when your new implementation comes out, though!

Sorry for the length of this post.
Ah, now I get it. Thanks for taking the time to write such a detailed answers.
Since you seem to want a rather fine-grain control over what is loaded and what not, my proposed solution does not give you the required flexibility. So, do not further bother about what I said.
Sure thing! And hey, your feedback still has a lot of great ideas in it. I can definitely incorporate the background thumbnail file generation into my changes, so that the files are still made, even if they aren't loaded yet. I think this should just be added to the thumbnail.save? No need to make another config setting, I think.

On that note, merging `max_ahead` and `max_behind` might still be a good idea. I'll defer to @karlch on this, I think, since he already agreed to the first way. I'm not particularly married to the idea of being able to have different amounts loaded ahead and behind, and it would be good to reduce config file complexity.

Will also definitely test changing these settings while the software is running.
I agree the points @jcjgraf made are useful, I see two points here:
- Independent of any changes: generate "visible" thumbnails first, instead of starting at the front. Would not mingle this with any changes, but do this in an additional PR, in case we go for it.
- Always generate thumbnails, independent of them being loaded in your case. If anything, this requires the `max_ahead` stuff, not `thumbnail.save`? Although it should probably check in an earlier stage, if they should be created at all, instead of running through all of them and then discarding, in case `thumbnail.save` is false :smile: This behaviour would then depend on both. Once again, I would prefer an extra PR, to avoid having monster-PRs :blush: 

Personally, I won't be using the `max_ahead` / `max_behind` stuff. So I also don't really have a preference between one or more settings. Random sample, have you ever set them to different values? :smile: 
> Independent of any changes: generate "visible" thumbnails first, instead of starting at the front. Would not mingle this with any changes, but do this in an additional PR, in case we go for it.

I agree, it is less related than I thought (well, code-wise they are, but not functionality-wise) and both changes are definitely complex enough on their own. I can deal with my idea once your PR is done.
I actually think the first point can readily be implemented by changing the sorting of the thumbnail list that's fed into the manager. The machinery to pass thumbnail paths in arbitrary order with their intended index is already there.

What I mean by the second point is, with `thumbnail.save`, perhaps the files associated with all thumbnails should be created, even if they aren't into the range to be displayed yet. So, it would better accommodate @jcjgraf's use case of slow-loading raw files.

Honestly, I've never tried setting `max_ahead` and `max_behind` to different values. I think. Switching the code to a single config value would be trivial.
Well, yes and no, depends on how far we want to go. Think of the following process:
1. `vimiv *.jpg` or whatever opens **a lot** of images
2. Thumbnails are instantly generated from the beginning in the current behaviour
3. We run `G` and `gt` to view the last images .. and wait. The manager is busy generating from the start. So we would have to interact in some way, stopping the process (which is async) and re-start it. This may get out of hand quickly.

If we know in advance, we want to start at the end, passing the images in reverse to image and using the new `none` sort option would have the same effect.

Anything in between just becomes a messy async communication, not sure we want to go into that rabbit hole.

I will have to take a closer look if the new `max_*` settings stop this in some way, but I fear anything that involves larger navigation would involve havoc, especially if we want to load "all" but "re-prioritize" on the fly.

EDIT:
forgot, let's go for one setting then instead of two here :+1: 
One config option it is! And, I get what you're saying, now. Basically, off the top of my head, I would probably try to implement the "create all thumbnail files but not load them" functionality by making an option for the thumbnail manager to not send the signal that new images have been loaded. So, the thumbnail will be created on the disk, but the load signal will only be sent when the image comes into range. I think the question to figure out is: What happens the image comes into range while a background thumbnail creation job is running?

Could alternately do it by having the thumbnail GUI component keep track of the paths it has sent for thumbnail file creation. If an image comes into range while its thumbnail is being made, it just lets the old job finish instead of starting a new one. All jobs send the creation signal back, but the signal is ignored unless it's in range.

Yeah, you're right, this is separate pull request material, though. It's not trivial.
Definitely extra PR material, agreed :smile: 

@jcjgraf could you clarify: I expect the extraction takes long, i.e. the generation of the initial `QPixmap`, loading or writing to disk really is instant afterwards? So we would have to change the order of the actual "generation" process, not just "not load them into the ThumbnailView"?

Fix patch:
diff --git a/AUTHORS b/AUTHORS
index 9c53b3886..4370bcaf1 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -3,6 +3,7 @@ Wolfgang Popp (woefe) <mail at wolfgang-popp dot de>
 Ankur Sinha (sanjayankur31) <ankursinha at fedoraproject dot org>
 Jean-Claude Graf (jcjgraf) <jeanggi90 at gmail dot com>
 Mateus Etto (Yutsuten) <mateus dot etto at gmail dot com>
+buzzingwires (buzzingwires) <buzzingwires at outlook dot com>
 
 All contributors of non-trivial code are listed here. Please send an email to
 <karlch at protonmail dot com> or open an issue / pull request if you feel like your
diff --git a/docs/changelog.rst b/docs/changelog.rst
index 3f98e4f8f..8c6717a97 100644
--- a/docs/changelog.rst
+++ b/docs/changelog.rst
@@ -89,6 +89,8 @@ Added:
   the changes with ``<return>`` and reject them with ``<escape>``. The
   ``{transformation-info}`` status module displays the currently selected geometry of
   the original image. Thanks `@Yutsuten`_ for reviving this!
+* Add the ``none`` sorting type for the ``sort.image_order`` and ``sort.directory_order``
+  options, implemented by `@buzzingwires`_
 
 Changed:
 ^^^^^^^^
@@ -535,3 +537,4 @@ Initial release of the Qt version.
 .. _@timsofteng: https://github.com/timsofteng
 .. _@kAldown: https://github.com/kaldown
 .. _@Yutsuten: https://github.com/Yutsuten
+.. _@buzzingwires: https://github.com/buzzingwires
diff --git a/docs/documentation/configuration/settings.rst b/docs/documentation/configuration/settings.rst
index a00eb5a1b..1412b0f91 100644
--- a/docs/documentation/configuration/settings.rst
+++ b/docs/documentation/configuration/settings.rst
@@ -56,9 +56,11 @@ directories. The ordering principle is defined by the ``sort.image_order`` and
    natural                   Natural ordering by basename, i.e. image2.jpg comes before image11.jpg
    recently-modified         Ordering by modification time (``mtime``)
    size                      Ordering by filesize, in bytes for images, in number of files for directories
+   none                      Do not sort or reverse. Use the existing order of the images (including that of a previous sort type). This is mostly for keeping the order from the command line or stdin.
    ========================= ===========
 
 In addition, the ordering can be reversed using ``sort.reverse`` and the string-like
 orderings (``alphabetical`` and ``natural``) can be made case-insensitive using
-``sort.ignore_case``. When ``sort.shuffle`` is set, the image filelist is shuffled
-regardless of the other orderings, but the library remains ordered.
+``sort.ignore_case`` except for when the ``none`` ordering type is used.
+When ``sort.shuffle`` is set, the image filelist is shuffled regardless of the other
+orderings, but the library remains ordered.
diff --git a/vimiv/api/settings.py b/vimiv/api/settings.py
index 9f77d7409..4d1d9c4c1 100644
--- a/vimiv/api/settings.py
+++ b/vimiv/api/settings.py
@@ -325,6 +325,7 @@ class OrderSetting(Setting):
         "alphabetical": str,
         "natural": natural_sort,
         "recently-modified": os.path.getmtime,
+        "none": lambda x: 0
     }
 
     STR_ORDER_TYPES = "alphabetical", "natural"


Test patch:
diff --git a/tests/end2end/features/image/imageorder.feature b/tests/end2end/features/image/imageorder.feature
index b06a758e8..1bb15b0a3 100644
--- a/tests/end2end/features/image/imageorder.feature
+++ b/tests/end2end/features/image/imageorder.feature
@@ -8,6 +8,26 @@ Feature: Ordering the image filelist.
         And the image number 2 should be image_2.jpg
         And the image number 11 should be image_11.jpg
 
+	Scenario: Set none sorting after another sort
+        Given I open 12 images without leading zeros in their name
+        When I run set sort.image_order natural
+        Then the image should have the index 1
+        And the image number 1 should be image_1.jpg
+        And the image number 2 should be image_2.jpg
+        And the image number 11 should be image_11.jpg
+        When I run set sort.image_order none
+        Then the image should have the index 1
+        And the image number 1 should be image_1.jpg
+        And the image number 2 should be image_2.jpg
+        And the image number 11 should be image_11.jpg
+
+    Scenario: Reverse none sorting
+        Given I open 5 images
+        When I run set sort.image_order none
+        Then the image should have the index 1
+        When I run set sort.reverse
+        Then the image should have the index 1
+
     Scenario: Reverse current filelist
         Given I open 5 images
         When I run set sort.reverse!
diff --git a/tests/unit/api/test_settings.py b/tests/unit/api/test_settings.py
index 75c7e7d1c..eb13b8074 100644
--- a/tests/unit/api/test_settings.py
+++ b/tests/unit/api/test_settings.py
@@ -189,6 +189,17 @@ def test_order_setting_sort(
     assert sorted_values == expected_values
 
 
+@pytest.mark.parametrize("reverse", [True, False])
+@pytest.mark.parametrize("ignore_case", [True, False])
+def test_order_setting_sort_none(monkeypatch, reverse, ignore_case):
+    values = ["a5.j", "A6.j", "a11.j", "a3.j"]
+    monkeypatch.setattr(settings.sort.reverse, "value", reverse)
+    monkeypatch.setattr(settings.sort.ignore_case, "value", ignore_case)
+    o = settings.OrderSetting("order", "none")
+    sorted_values = o.sort(values)
+    assert sorted_values == values
+
+
 @pytest.mark.parametrize("ignore_case", [True, False])
 def test_order_setting_sort_ignore_case(monkeypatch, ignore_case):
     monkeypatch.setattr(settings.sort.ignore_case, "value", ignore_case)