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

Select root folder for timeline #515

Closed
wants to merge 1 commit into from

Conversation

Mikescops
Copy link
Member

Closes #141

Signed-off-by: Corentin Mors <corentin.mors@dashlane.com>
@Mikescops Mikescops added enhancement New feature or request 3. to review Waiting for reviews feature: timeline Related to the timeline section labels Oct 23, 2020
@Mikescops Mikescops self-assigned this Oct 23, 2020
@Mikescops
Copy link
Member Author

@skjnldsv I don't know why but .jpg (which are JPEG files) are not taken in account, for some reasons, and as of now i can't find why..

@Mikescops
Copy link
Member Author

Seems like the dav:or is not working and only the first mime type is taken in account.

@strugee
Copy link
Member

strugee commented Oct 24, 2020

This only provides for one root folder, right? That would be a problem for me and I assume other folks too since there's a couple places photos go if you use the out-of-the-box defaults - brand new accounts have a "Photos" folder created for them, so that's a natural place to put them, but the Android app auto uploads photos to the "InstantUpload" folder by default.

@ntimo
Copy link

ntimo commented Oct 24, 2020

But when there is no photos directory select manually the app should function as it does with the default settings, I would assume. So this would also some your issue @strugee. Am I correct @Mikescops? :)

@strugee
Copy link
Member

strugee commented Oct 24, 2020

@ntimo right, that would be a workaround. But then I have the original issue described in #141 again.

@Mikescops
Copy link
Member Author

By default no difference with the current version, but you can select one folder from where images on your timeline will be fetched from.

Comment on lines +74 to +80
? `<d:eq>
<d:prop>
<oc:owner-id/>
</d:prop>
<d:literal>${getCurrentUser().uid}</d:literal>
</d:eq>`
: ''
Copy link
Member

Choose a reason for hiding this comment

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

This is needed to not have the shares as results.
Why did you need to put a condition on it?
It should not cause any issues like it was before :)

Copy link
Member Author

Choose a reason for hiding this comment

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

The owner filter is only allowed on the root. Dav throws this error if you try on subfolders. What do you think then?

Copy link
Member

Choose a reason for hiding this comment

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

Then it should be fixed on server.
cc @rullzer and @icewind1991 :)

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

$this->initialStateService->provideInitialState($this->appName, 'croppedLayout', $this->config->getUserValue($user->getUid(), Application::APP_ID, 'croppedLayout', 'false'));
$this->initialStateService->provideInitialState($this->appName, 'timelineRootFolder', $this->config->getUserValue($user->getUid(), Application::APP_ID, 'timelineRootFolder', '/'));
Copy link
Member

Choose a reason for hiding this comment

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

We should probably limit the allowed keys here:

$user = $this->userSession->getUser();

As a safety mesure 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure what you mean

Copy link
Member

Choose a reason for hiding this comment

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

setUserConfig allows anything, and will put it straight into the db.
Meaning we could override existing values. We should check if the $key is either timelineRootFolder or croppedLayout and throw otherwise as a safety measure

Copy link
Member Author

Choose a reason for hiding this comment

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

ok gotcha

@strugee
Copy link
Member

strugee commented Oct 31, 2020

By default no difference with the current version, but you can select one folder from where images on your timeline will be fetched from.

@Mikescops right, to clarify, I'm not suggesting this would break existing workflows - I'm just saying that the feature might not actually be complete enough for many(?) users due to defaults in the server/Android app. Speaking only for myself, for example, I would not be able to use this feature since given the way my Nextcloud is organized, if I turned it on I would end up missing a ton of images that I'd want to see in the Photos app.

@jancborchardt I'd be interested in hearing if you agree or if you think it's not worth the additional interface/code complexity.

@Mikescops
Copy link
Member Author

It's outside of the scope of this PR, if you're not happy with it, open a separate issue after it's merged, thanks.

@saywebsolutions
Copy link

Which release will this go into? I'm looking forward to being able to use photos again, thank you @Mikescops

@Mikescops
Copy link
Member Author

NC 21 at least, for now we're stuck with a server issue.

@StarSmasher44
Copy link

Hi! Sorry to bring this back, but I am hopeful this merge will resolve my issue. Is there any status update on this?
I saw the 'server problem' pull had been merged, is there anything else that needs to be done or fixed in order to get this in asap?

@skjnldsv
Copy link
Member

I saw the 'server problem' pull had been merged, is there anything else that needs to be done or fixed in order to get this in asap?

Hey! The server hasn't received anything regarding this fix yet. Unfortunately this is not yet possible :/

@StarSmasher44
Copy link

I saw the 'server problem' pull had been merged, is there anything else that needs to be done or fixed in order to get this in asap?

Hey! The server hasn't received anything regarding this fix yet. Unfortunately this is not yet possible :/

Ah well that sucks, I was referring to this one: nextcloud/server#17941 mentioned some posts up.

But would you happen to know what still needs to be done/added/fixed to make this work? I have a limited background in web development languages so I might be able to pull something through, even though I highly doubt it.

@skjnldsv
Copy link
Member

But would you happen to know what still needs to be done/added/fixed to make this work? I have a limited background in web development languages so I might be able to pull something through, even though I highly doubt it.

It require a fair amount of rework on the backend dav search implementation :/

@StarSmasher44
Copy link

But would you happen to know what still needs to be done/added/fixed to make this work? I have a limited background in web development languages so I might be able to pull something through, even though I highly doubt it.

It require a fair amount of rework on the backend dav search implementation :/

Sorry for being overly asking questions or coming off as pushy in any regard, I am merely just trying to figure out a good entry point, I looked through the code twice but I can't find an obvious stopping point beside the mentioned error with single vs multiple mime types being used.

And I am unsure if you even know enough of this PR yourself to respond (It isn't easy going off using other people's work as a starting point for yourself, imo) but I still have no idea what is actually wrong with this code, what needs fixing and what doesn't.

To me it looks mostly fine, given all it really needs to do is enter/search a single folder and mark that as the 'Photos' folder for NextCloud, if I get this correct.

If I have more information I may try to play around with this to see if I can bring it to a working state somehow, I have a bad taste for using OneDrive as my image backup server ever since Samsung forced it upon me, and this still adds in 600 images from my several Unity projects.

@skjnldsv
Copy link
Member

skjnldsv commented Jan 7, 2021

@StarSmasher44 I suggest you read the original issues like nextcloud/server#17941 #141 entirely (with the folded comments too)

Again, this requires a fix on server. Currently there is a hard limitation for our webdav SEARCH that forces you to limit the search to a user home for performance issues.
That is why we don't have external storage nor groupfolders. And if you ignore this, you will not be able to properly filter it anymore. This requires a big revamp for al the following points to be implemented:

  • Allow to search/filter by tags (favourites)
  • Allow to search/filter by mime
  • Allow to search/filter by user
  • Allow to search/filter within a folder OR from root (currently only root is allowed with filtering)
  • Allow to limit results
  • Allow to sort results

Currently, the combo search-from-folder AND filter-by-owner is missing. It's not enabled (hard-coded on server) Once it is here, we'll be able to tell the Photos app to search from within the provided root folder instead of the default root.

@timsayshey

This comment has been minimized.

@skjnldsv skjnldsv added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Feb 17, 2021
@blizzz blizzz added this to the Nextcloud 25 milestone Apr 21, 2022
@blizzz blizzz modified the milestones: Nextcloud 25, Nextcloud 26 Sep 22, 2022
@blizzz blizzz modified the milestones: Nextcloud 26, Nextcloud 27 Mar 9, 2023
@skjnldsv skjnldsv removed this from the Nextcloud 27 milestone May 10, 2023
@skjnldsv skjnldsv marked this pull request as draft May 10, 2023 09:47
@AndyScherzinger AndyScherzinger added this to the Nextcloud 29 milestone Feb 20, 2024
@AndyScherzinger
Copy link
Member

@artonge I guess this PR is obsolete with #2319 having been merged?

@artonge artonge closed this Mar 21, 2024
@jancborchardt jancborchardt deleted the feature/select-root-folder-timeline branch May 7, 2024 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress enhancement New feature or request feature: timeline Related to the timeline section
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🖼️📂 Limit Photos to a single folder + subtree
10 participants