Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issue 1332 (photos from external folders not shown) #1371

Conversation

umgfoin
Copy link
Contributor

@umgfoin umgfoin commented Jan 9, 2025

GeophotoService::getAll tries to load filecache-entries by id for each photo-entity in the photomapper.
Since NC30 this regularly fails for images located on ext. storage: As a result, the corresponding photos won't get included in the distributed photosCache and thus don't show up on the map.
I yet don't see the root-cause for the cache-failures here, but imho, access to the filecache is not needed - neither does it provide any performance gain. I decided to remove the corresponding filecache-code and retrieve the required attributes (Etag and mimetype) from the anyway fetched file-object.

$filesById = [];
$cache = $folder->getStorage()->getCache();
$previewEnableMimetypes = $this->getPreviewEnabledMimetypes();
foreach ($photoEntities as $photoEntity) {
        $cacheEntry = $cache->get($photoEntity->getFileId());
        if ($cacheEntry) {
                [...]
                $filesById[] = $file_object;
        }
        $this->photosCache->set($key, $filesById, 60 * 60 * 24);

This fixes #1332

Other changes:

  • injection of LoggerInterface into GeophotoService.
    - A false failure-message when clearing the photo-cache was fixed.

@umgfoin umgfoin force-pushed the fix-issue-1332-(photos-from-external-folders-not-shown) branch 2 times, most recently from 762ea61 to a2bab8e Compare January 9, 2025 23:57
@umgfoin umgfoin force-pushed the fix-issue-1332-(photos-from-external-folders-not-shown) branch 2 times, most recently from 2cfffa3 to 2dcfb7e Compare January 10, 2025 09:20
@umgfoin umgfoin requested review from tacruc and come-nc January 13, 2025 10:01
@umgfoin umgfoin requested a review from julien-nc January 13, 2025 21:24
@umgfoin umgfoin force-pushed the fix-issue-1332-(photos-from-external-folders-not-shown) branch 2 times, most recently from 99ea945 to 8c34ea8 Compare January 16, 2025 07:44
Copy link
Collaborator

@tacruc tacruc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@umgfoin thanks for your contribution. Unfortunately my dev setup is currently not running. Did you notice any performance Impacts with this change?
Do you think we should create an option to enable and disable external storages? Or can this be done via .nomedia files anyway? What do you think?

lib/Service/GeophotoService.php Outdated Show resolved Hide resolved
@umgfoin
Copy link
Contributor Author

umgfoin commented Jan 16, 2025

Did you notice any performance Impacts with this change?

For me not noticeable: Loadtime for ~8K photos with empty photoCache till initial display: 15s
With warm photoCache 5s.
I think the changes have no performance impact at all: We're just using already present information instead of additionally querying the filecache for Etag and Mimetype:
$files = $folder->getById($photoEntity->getFileId()); is already fetching Etag and mimetype.

Do you think we should create an option to enable and disable external storages? Or can this be done via .nomedia files anyway?

The .nomedia exclusion-switch seems to be respected for external folders - I see no need for an explicit option.

@umgfoin
Copy link
Contributor Author

umgfoin commented Jan 16, 2025

@come-nc & @tacruc
I agree to discuss the clearcache-error in a separate PR.
What do you think of something like this:

public function clearCache(string $userId = ''): bool {
    try {  
                $this->photosCache->clear($userId);
                $this->timeOrderedPointSetsCache->clear($userId);
                $this->backgroundJobCache->clear('recentlyAdded:'.$userId);
                $this->backgroundJobCache->clear('recentlyUpdated:'.$userId);
                return true;
    } catch (\Exception $e) {
                return false;
    }
}

This would NOT treat clearing an empty cache as an error, but still give a visual GUI-feedback, if something abnormal happened.

@umgfoin umgfoin force-pushed the fix-issue-1332-(photos-from-external-folders-not-shown) branch 2 times, most recently from 8258635 to a2e54d7 Compare January 16, 2025 14:20
Copy link
Collaborator

@tacruc tacruc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@umgfoin Let's go.

Remove FileCache-usage in GeophotoService::getAll and GeophotoService::getNonLocalized

Signed-off-by: umgfoin <umgfoin@users.noreply.github.com>
Signed-off-by: umgfoin <umgfoin@users.noreply.github.com>
Signed-off-by: umgfoin <umgfoin@users.noreply.github.com>
Signed-off-by: umgfoin <umgfoin@users.noreply.github.com>
@umgfoin umgfoin force-pushed the fix-issue-1332-(photos-from-external-folders-not-shown) branch from a2e54d7 to 90689cc Compare January 17, 2025 08:23
@umgfoin umgfoin enabled auto-merge January 17, 2025 08:27
@umgfoin umgfoin merged commit 78a4d9b into nextcloud:master Jan 17, 2025
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Photo from external folder are scanned but not displayed
3 participants