LycheeOrg / Lychee-front

JS implementation of Lychee frontend
https://lycheeorg.github.io/
MIT License
48 stars 53 forks source link

Display takedate for photos and albums #241

Closed evoludolab closed 3 years ago

evoludolab commented 3 years ago

This pull request addresses a few minor issues with the front-end:

  1. When sorting albums by anything but timestamps (e.g. by title), the date below the title shows the import date. Regardless of sorting preferences it seems preferable to display the date range of photos in the album (build.album, l.84-89).
  2. Photo views show no overlay if EXIF info is missing. In these cases the overlay should at least show the title and the creation/upload date (build.overlay_image, l.253ff)
  3. For media files with corrupt/incomplete EXIF information a few checks are performed while compiling the overlay to prevent empty fields (build.overlay_image, l.275ff)
evoludolab commented 3 years ago

This is an interesting suggestion. In effect, it would be more consistent to what we already do for individual photos, for which we always display the takedate if available. Perhaps we should also distinguish the takedate from sysdate using the little camera icon that we already use for this purpose for photos?

I like this idea and will look into it.

As an aside, min_takedate and max_takedate are based on takedate, i.e. the EXIF info in media files. However, based on LycheeOrg/Lychee#874 sysdate may now refer to the timestamp of imported media files if no EXIF info is available but is not reflected in min/max_takedate. Anyways, I think the status quo is fine because media files with missing EXIF should be the exception and in cases where the timestamp of the media is meaningful, it remains unclear how this information could be incorporated min/max_takedate of the album in a transparent manner. In contrast, the sysdate of the album will always be its creation date, right?

I'm a little worried though that we may get negative feedback about making this change unconditionally, as it is a pretty visible one (see also https://xkcd.com/1172/ 😉). It would be easier to swallow if takedates were editable, but that's still only on my todo list.

Great reference - I'll put in a feature request for a space-bar-heater 😉 Of course I agree. Clearly, the proper approach is to keep the sorting of albums and the information to display as a subtitles separate and controlled by separate configuration settings. I'll look into it and see how far I get...

I haven't tested this but see my comment in the code about a possible breakage. BTW, are you aware that you can switch between the description types while viewing a photo using the o keyboard shortcut? Also, I would consider adding the little camera icon here as well.

I didn't know about the o - thanks for pointing out! Indeed this results in a duplication of dates in some cases. This is easy to fix, though.

evoludolab commented 3 years ago

The album subtitle is now configurable to maintain space-bar-heater workflows 😉 (and by default subtitle format unchanged), the camera icon is added to mark EXIF dates and the formatting of the photo overlay is fixed.

Instead of opening another pull request on the back end I copy the migration file for adding the new parameter here database/migrations/2021_01_24_103000_config_album_subtitle_type.php:

<?php

use App\Models\Configs;
use Illuminate\Database\Migrations\Migration;

class ConfigAlbumSubtitleType extends Migration
{
    /**
     * Run the migrations.
     *
     * @return void
     */
    public function up()
    {
        DB::table('configs')->insert([
            [
                'key' => 'album_subtitle_type',
                'value' => 'oldstyle',
                'confidentiality' => '0',
                'cat' => 'Gallery',
                'type_range' => 'description|takedate|creation|oldstyle',
            ],
        ]);
    }

    /**
     * Reverse the migrations.
     *
     * @return void
     */
    public function down()
    {
        Configs::where('key', '=', 'album_subtitle_type')->delete();
    }
}
ildyria commented 3 years ago

I did not check it yet yet but please verify my code simplification (which should be easier to follow in term of cognitive complexity).

evoludolab commented 3 years ago

Thanks @ildyria. I agree, your code is shorter, simpler and achieves the same. In general I like switch-statements but in this case the continue and unreachable parts made the logic indeed less accessible. The only quibble is that exifHash is now always built, even if unused. This could be solved with a nested if but the overhead is negligible.

kamil4 commented 3 years ago

Sorry for going silent on this; I'm hoping to be able to give my feedback later today. I was dealing with my own PR that was giving me no end of troubles but thankfully that's out the door now so I should finally be able to focus on others'.

kamil4 commented 3 years ago

I pushed some minor tweaks:

kamil4 commented 3 years ago

I ran the database migration so that I could enable the new album subtitle types and I must say that I really like how albums look now! I just can't help wondering why none of us thought of doing this earlier? Well, I guess those of us who sort albums by min/max takestamp did, but without the camera icon being displayed. :smiley:

sonarcloud[bot] commented 3 years ago

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication