Implement file sorting in thumbnail view

[haasn]

For simplicity, I decided to only implement the attributes that I could
verily easily reach from the existing metadata exported by Thumbnail -
which is the name (as usual), the datetime string (which should
effectively be the same as sorting by the actual date), the rank, and -
because it was there - the EXIF line. Ideally, I would also like to be
able to sort by "last modified" but I'm not sure of the best way to
reach this from this place in the code.

It's worth pointing out that, with the current implementation, the list
will not dynamically re-sort itself until you re-select the sorting
method - even if you make changes to the files that would otherwise
affect the sorting (e.g. changing the rank while sorting by rank). One
might even call this a feature, not a bug, since it prevents thumbnails
from moving around while you're trying to re-label them. You can always
re-select "sort by ..." from the context menu to force a re-sort.

Fixes #3317

[haasn]

[haasn]

For reference, I modeled the design after the pcmanfm context menu, which looks similar. Only difference is that it also has small radial ("option" style) checkboxes next to each option highlighting which one is currently in effect. That would be nice-to-have for us as well, but I got lazy. Maybe on a rainy day.

[Lawrence37]

Good to see someone working on the file browser sorting!

Regarding the automatic re-sorting, the current file browser will re-sort after a file name change. The image actually disappears then reappears, but it gets placed in a new location. I do agree that it may be better to not update the order after changing the rating or color label.

It seems you are using the date-time string that appears below the thumbnail? This might be problematic because the date format can be changed from the default year-month-day, so the sorting won't work as expected if it is set to month-day-year, for example.

Possible improvements either in this PR or in the future:

• Consider the time zone. This can be challenging because each make puts the time zone in a different place or excludes it all together.
• Add sorting widgets to the top toolbar so it is easy to discover and use.

[haasn]

Re: date sorting I think the proper solution would be to convert everything to UTC (e.g. via Glib::DateTime intermediary) for comparison sake. I'll have to look into how the plumbing for that works, so I can ideally bypass the string altogether.

I'll probably continue hacking at it $tomorrow or something, mostly submitted this to start getting feedback in and because I reached a limit of how much I wanted to explore this codebase for a day. Definitely also plan on adding a widget to the toolbar, should be easy enough to make an icon for it (the usual one is two arrows pointing in opposite directions).

[haasn]

Okay, I got some more time to hack on this. I made the following changes:

1. Stop sorting based on the datetime string and start propagating a Glib::DateTime directly, to get around string-sorting limitations.
2. Implement metadata plumbing for timezone information (as a string), which we can pull from EXIF (WIP: still need to do the actual EXIF loading/writing)
3. Stop manually generating the DateTime string in favor of using DateTime::format, which allows the use of all recognized date/time formats.

1 is a clear improvement, 2 is a work-in-progress, and 3 is something I'm not sure you actually want. (Since it's not backwards compatible, and also the translations need to be updated...)

[Lawrence37]

For 3, we can check the program version in the options file or use a new key to store the new format, and convert from the old format one time.
By the way, the AppImage build failed. Maybe there's a missing #include.

[haasn]

By the way, the AppImage build failed. Maybe there's a missing #include.

Yeah, needs an #include <timezone.h> as well. Should be fixed now.

[haasn]

So, I wanted to look into this again (specifically the timezone stuff) but it seems that at least my camera doesn't actually write any tags that could be used to determine the timezone from. So for the time being I think I'll scrap this approach/commit and simply go back to storing only the timestamp itself, without timezone information.

That also means we can (and probably should) split off the UI display change to a separate commit, if it's even desired at all.

[haasn]

Okay, I dropped the EXIF stuff for now and just implemented the sorting commit. I also switched from bare items to radio items, massively increasing quality of life in my mind.

I consider this commit now ready as-is.

[haasn]

Sorry for the delay. Should be fixed and updated now, and I confirmed that it works as expected in a mixed folder of valid, invalid and untagged (EXIF-less) image files, with a silent fallback to the modification date for EXIF-free files.

[Lawrence37]

Windows and Linux builds are failing...

https://github.com/Beep6581/RawTherapee/runs/6436307468?check_suite_focus=true#step:5:93

D:/a/RawTherapee/RawTherapee/rtengine/imagedata.cc:109:5: error: 'gmtime_r' was not declared in this scope; did you mean 'gmtime_s'?
  109 |     gmtime_r(&timeStamp, &time);
      |     ^~~~~~~~
      |     gmtime_s

https://github.com/Beep6581/RawTherapee/runs/6436307448?check_suite_focus=true#step:5:331

/home/runner/work/RawTherapee/RawTherapee/rtgui/thumbnail.cc:753:9: error: no match for ‘operator!’ (operand type is ‘Glib::DateTime’)
     if (!dateTime || !cfs.timeValid) {
         ^~~~~~~~~

[haasn]

/home/runner/work/RawTherapee/RawTherapee/rtgui/thumbnail.cc:753:9: error: no match for ‘operator!’ (operand type is ‘Glib::DateTime’)
     if (!dateTime || !cfs.timeValid) {
         ^~~~~~~~~

I'm not familiar with C++. What's a good way of working around this? Should I use if (!static_cast<bool>(dateTime) || !cfs.timeValid)?

Edit: I suppose easiest is to just invert the condition to if (!(dateTime && cfs.timeValid))

[Lawrence37]

I don't think Glib::DateTime can be cast into a bool. The class needs an operator bool() which is only present in glibmm >=2.62.0. I don't see any other method to check if the datetime is valid. The workaround is probably to get the underlying GDateTime with gobj() and check if it's null.

[Floessie]

Your could try to use Glib::DateTime::equal() with a default constructed object (i.e. if (dateTime.equal({}))). Don't know if this works, but seems logical...

[haasn]

The workaround is probably to get the underlying GDateTime with gobj() and check if it's null.

Undefined behavior on nullptr.

Edit: ah, no, it doesn't actually return a nullptr, it returns a DateTime that has gobject_ set to a nullptr. So yeah that should work.

[Lawrence37]

Wonderful. I will give this another test run soon. It looks like it should work fine, although the gmtime issue still looks suspicious.

[Lawrence37]

Tested on Linux and it looks good. The modified times show up in UTC 0, but it's an improvement compared to what we currently have (incorrect date or no date at all). This and the EXIF time zone issues can be tackled later. If someone can confirm these changes work on Windows, I'm in support of merging for 6.0.

[Lawrence37]

I just tried it in Windows and it looks ok.

[fatso83]

Just bumping this since it seems to have disappeared from view and looks merge-ready (I don't see any negatives not addressed?) and is highly sought after.

[Lawrence37]

@fatso83 The PR is indeed complete, but the dev branch is basically locked in preparation for the upcoming release. I anticipate this PR (and several others) to be merged shortly after the release.

[fatso83]

@Lawrence37 Appreciate that info. Coming from the (nowadays) more common web release process (CI), one forgets about the normal static software release processes. Might be worth mentioning in the contributing guide?, though on second thought, I am not sure it would avoid drive by comments like mine 🙈.

[haasn]

Ping

[fatso83]

Workflows are usually set up as not to have releases block merging to the dev branch. Normal Git Flow solves this perfectly for versioned software.

1. Branch release/v1234 off development
2. Stabilize release/v1234, continuously backmerging fixes to development
3. Release release/v1234 once deemed stable, tag and delete the branch

All while merging features into development (and/or daggy fixes).

[Thanatomanic]

While this branch seems ready to merge, it will generate quite some conflicts for our exiv2 branch. This would make merging even harder... Ping @Lawrence37 for their opinion. Should we wait to merge this?

[Lawrence37]

We will have to deal with the merge conflicts eventually. Cherry-picking these commits into the exiv2 branch reveals the conflicts are not too difficult to resolve. I'm ok with merging this PR.

[Thanatomanic]

Thanks @haasn for your contribution and @fatso83 for commentary. And both for your patience...