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

Scan photos of specific directory #1231

Merged
merged 17 commits into from
Dec 21, 2024
Merged

Scan photos of specific directory #1231

merged 17 commits into from
Dec 21, 2024

Conversation

tetebueno
Copy link
Contributor

@tetebueno tetebueno commented Mar 27, 2024

I've been getting memory exhaustion when scanning all photos collection on an initial load; trying to address #525 I made these changes and was able to scan each year/month directory of photos succesfully like this (the example is for the folder with photos of july '84):

$ docker exec -t -u www-data container-name php occ maps:scan-photos -vvv username Photos/1984/06

I didn't like the ifs all over the implementation, but it's something like a make it work approach. If I can get comments about it, we can make it better.

Cheers.

@tacruc
Copy link
Collaborator

tacruc commented Mar 27, 2024

Thanks for your contribution. Can you pass the path as optional argument?
Do you need help to fix the DCO?

@tetebueno
Copy link
Contributor Author

You are very welcome.

Done with DCO, thanks.

Is there anything else besides this so the path parameter is optional? I believe it already is since help command displays it like this: ... [<user_id> [<path>]].

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.

Hi @tetebueno,
sorry for the late reply and beeing a little unprecise.
I mean in stead of adding the functions
RescanPhotos->scanUserPhotos
and
PhotofilesService->rescanPath

Just provide an optional parameter to

       private function rescanUserPhotos(string $userId, bool $inBackground=true, sting? $pathToScan===null) {

and PhotofilesService
public function rescan($userId, $inBackground=true, $pathToScan=null) {

and then make an if in the Photofilesservice->rescan

$userFolder = $this->root->getUserFolder($userId)
if ($pathToScan === null) {
          $folder = $userFolder
} else {
          $folder = $userFolder->get($pathToScan);
}
$photos = $this->gatherPhotoFiles($folder, true);

Don't blindly copy my code. It is just typed in here to outline my thought and not tested.

lib/Command/RescanPhotos.php Outdated Show resolved Hide resolved
lib/Command/RescanPhotos.php Outdated Show resolved Hide resolved
@tacruc
Copy link
Collaborator

tacruc commented Apr 21, 2024

@tetebueno Do you think you will make the changes befor the NC29 release. Then I would wait with #1244 to include this.

I'm sorry that it is on such short notice after not reviewing for a month. Just have to decide whether to wait or not.

@marlemion
Copy link

I just wanted to bring this back to mind. In my case it is tedious to update ~100000 Pictures just to get some recently geotagged pictures into Maps....

@tetebueno
Copy link
Contributor Author

Apologies to everyone. Totally forgot about this.

@tacruc, I'm not completely sure what does this line do in RescanPhotos+rescanUserPhotos(), but I tried to maintain the logic intended in the first PR version:

$this->config->setUserValue($userId, 'maps', 'installScanDone', 'yes');

Signed-off-by: tete <[email protected]>
Signed-off-by: tetebueno <[email protected]>
Signed-off-by: tete <[email protected]>
Signed-off-by: tetebueno <[email protected]>
@tetebueno
Copy link
Contributor Author

Forgot DCO again and it seems I went too far with it double-signing-off everything.

@tetebueno tetebueno requested a review from tacruc November 22, 2024 20:40
@tetebueno
Copy link
Contributor Author

tetebueno commented Nov 24, 2024

Ok, my bad, tried merging with Github's visual tool and it was an indentation mess challenge. Will take care of this as soon as I can.

nextcloud-bot and others added 8 commits November 29, 2024 19:43
Signed-off-by: Nextcloud bot <[email protected]>
Signed-off-by: tetebueno <[email protected]>
Signed-off-by: Nextcloud bot <[email protected]>
Signed-off-by: tetebueno <[email protected]>
Signed-off-by: Nextcloud bot <[email protected]>
Signed-off-by: tetebueno <[email protected]>
Signed-off-by: Nextcloud bot <[email protected]>
Signed-off-by: tetebueno <[email protected]>
Signed-off-by: Nextcloud bot <[email protected]>
Signed-off-by: tetebueno <[email protected]>
Signed-off-by: Nextcloud bot <[email protected]>
Signed-off-by: tetebueno <[email protected]>
Signed-off-by: Nextcloud bot <[email protected]>
Signed-off-by: tetebueno <[email protected]>
@tetebueno
Copy link
Contributor Author

Finally got some time to pull this to a local environment and merge and fix formatting issues. Hope this is it.

@danotrampus
Copy link

Also waiting for this... Any news?

@tacruc
Copy link
Collaborator

tacruc commented Dec 21, 2024

@tetebueno Thanks alot for your contribution. I will see if I find time the next two weeks to publish them. But I cannot promise it.

@tacruc tacruc merged commit 9cd6204 into nextcloud:master Dec 21, 2024
17 of 18 checks passed
@tetebueno
Copy link
Contributor Author

tetebueno commented Dec 21, 2024

Gaaa, just got a chance to run composer run cs:fix and pushed the fixed code. Thanks anyway. I'm available for any issues regarding this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants