scubajeff / lespas

Les Pas, photo album app for Nextcloud user
Apache License 2.0
454 stars 22 forks source link

Option to add remote albums that are already in nextcloud #112

Open 0verEngineer opened 1 year ago

0verEngineer commented 1 year ago

Is your feature request related to a problem? Please describe. I have a a bunch of albums on my nextcloud and it would be cool to browse them from my phone.

Describe the solution you'd like An option to add remote albums that do not live in the lespas directory structure.

scubajeff commented 1 year ago

You can copy or symlink folders to lespas's home folder. The next time you open lespas app, those folders will be synced to your phone as albums.

0verEngineer commented 1 year ago

Hey, thanks for your fast reply, that does already sound promising, however lespas does not use the directory i select, it creates a lespas directory inside which i cannot edit because it is greyed out. Why is this the case?

So this means that this feature request transforms to: Let the users choose lespas directory instead of creating a lespas named directory inside

scubajeff commented 1 year ago

The fixed name is there for a reason, Nextcloud's Share API treats all the shares a user made as a single image, e.g., you just can't tell which app shared which file/folder from the API output. So Les Pas needs this fixed folder name as a differentiator.

0verEngineer commented 1 year ago

Ok so i guess lespas needs to know which stuff was shared by itself and not by the user in the nextcloud ui?

Where is the difference if i can choose whatever directory i want?

In both cases i am able to just create a new share in the nextcloud ui in the lespas home directory, no matter the directory name.

Or did i get this wrong?

I mean this is such a beautiful app and i will definitely use it, but a normal user with no background info will just assume that he can point lespas to a directory in nextcloud where all his photos are stored. And in my opinion should this be the case usability wise.

scubajeff commented 1 year ago

If you have some idea of how web server works, you can try out Nextcloud's OCS Share API by yourself. Just curl the Get All Shares API and you should understand what 'Single image of Shares' is and how it will affect apps like Les Pas when dealing with sharing. Sorry, I just can't explain this clearly.

The hard coded lespas is simply NOT a silly logo to show off.

0verEngineer commented 1 year ago

Hey, i never said that the name is a show off. But it is not user friendly.

I played with the Share API and i get a normal xml or json answer with some shares inside, i don't get what you mean by 'Single image of Shares''.

I checked your code and you use the query param path like this (assuming Pictures is the selected dir, resulting in Pictures/lespas as lespas root dir): ?path=Pictures/lespas to get all the shares created by lespas.

Lespas assumes all shares in it's directory (Pictures/lespas) are created by lespas, but in fact nothing is blocking me from creating a share in the Nextcloud UI inside the lespas directory to break this assumption. The same is true for the Pictures directory.

So once again i'm asking what would be the difference when we use Pictures instead of Pictures/lespas as directory?

It seems to me that it would behave exactly the same no matter how the directory is named, it just breaks the user experience.

scubajeff commented 1 year ago

Nextcloud return a single file with all the shares you received, within this xml/json file, no way to tell which app shared which resource, it's a single image of your shares map.

So without the help of hard-coded folder name, to find out which one is shared by Les Pas, app need to look inside each shared folder the user received, one by one with several http calls each to determind it's origin. Not only time consuming but also not friendly in mobile environment.

0verEngineer commented 1 year ago

Ok again: Even with this hardcoded directory name you cannot tell that a specific share inside this directory is created by lespas because i can just create a new one in this directory without using lespas and lespas would just assume it was created by itself....

And lespas would not need to look in every directory, just in the root directory selected, so there is also no difference.

So there would be no difference if you let the user select the directory they want.

Everything would be exactly the same besides the directory name.

scubajeff commented 1 year ago

If you create the folder inside lespas/, Les Pas will happily see it as a new album. If the folder is created along side lespas/, no.

Les Pas also doesn't maintain the user/group relationship map, it's Nextcloud server's job. So it will never know which home folder the other user choosed. All it has it's this hard-coded name in the path.

0verEngineer commented 1 year ago

Your answer has nothing to do with my question.

I stand my point, the directory name is irrelevent and the user should be able to choose the directory the user wants. It would not make a difference for lespas since it has exactly the same infos as before: its own root directory.

"So it will never know which home folder the other user choosed. All it has it's this hard-coded name in the path." -> Even with the hard-coded name lespas is not able to tell which directory another user has chosen because the other use can have its lespas directory in different subdirectories.

I don't understand why you don't get this, i would like to try this myself and proof to you that it works, can you please list the features you think will not work?

scubajeff commented 1 year ago

Please imagine this, if user A choose a 'folderA' as it's home, and shared 'folderA/albumA' to user B, how would user B know this is the folder he is interested in if he has no idea what user A's home folder name?

0verEngineer commented 1 year ago

Ok, so in your example the root directory of userA is folderA/lespas, how does userB know the full path if he only knows the subdirectory name lespas?

scubajeff commented 1 year ago

The received share is in the "share_with_me/albumA" folder. 'share_with_me' is defined in server's config.php file. Have you ever tried sharing and/or received shares?

0verEngineer commented 1 year ago

Hey, sadly you did not answer my question.

My share_with_me is defined as /

I have just tried sharing again and i go with your example once again:

UserA shares folderA/lespas/share1 with userB, userB now has a share1 directory in the root of his nextcloud. Then i query all shares from userB and there is not one field in the json that has something to do with lespas or the path folderA/lespas/share1 (this is the same for joint and solo shares) so first of all i fail to see why userB needs to know the directory name as you use the SHARED_WITH_ME_ENDPOINT endpoint for loading shared items, the PREVIEW_ENDPOINT for loading the preview i assume. And then you can just load the file from the path the SHARED_WITH_ME_ENDPOINT gives back, there is no need to know the directory structure of userA.

And the problem that i want you to see is the same as before: userB can not know the exact location in userA because userA can set the path for the root directory, so why do you keep telling me that the reason for this directory naming is so that userB can know where the files live if there is no way that userB will know.

Can you please address both of this requests?

Thanks a lot

scubajeff commented 1 year ago

ok, now you are on the right track. you have to think from user B's point of view. what he knows is a whole bunch of files, folders in his 'share_with_me' and the result from calling the list shares endpoint. these are the only two pieces of information he can get from the server. how would he decide which folders are the interested ones? you should look close in the json file for the answer.

0verEngineer commented 1 year ago

So thanks again for your half baked answer.

I just checked everything and it is the same as before this directory name is not needed

I share a directory in lespas with userB, lespas creates the share and puts a json file inside

  1. userB's app uses the ocs/v2.php/apps/files_sharing/api/v1/shares?format=json&shared_with_me=true to get the list of shares.

  2. userB's app uses remote.php/dav/files/userB/shareName to get all the names of the items in the shared directory

  3. userB's app uses remote.php/dav/files/userB/shareName/194554-content.json to query the lespas share info json

  4. The app uses: webDav.getStream("${resourceRoot}${share.sharePath}/${share.albumId}.json", true, CacheControl.FORCE_NETWORK).use { JSONObject(it.bufferedReader().readText()).getJSONObject("lespas").let { meta -> to check if the first json object name inside the json is lespas

  5. userB sees the name lespas in the json and knows that this directory is shared by lespas

The directory structure of userA is never used nor known in the whole process, in fact it would be a security problem if userB would be able to see userA's directory structure.

scubajeff commented 1 year ago

Your step 2 and step 3 are exactly what the current implementation try to avoid, which is scanning all the received shared folders.

All information on shares are from API call result, so I would argue that there are no security issue what so ever.

scubajeff commented 1 year ago

You might have a point here. I need to revisit my code, since most of the codebase concerning sharing are more than one year old. Give me several days, I'm not with my dev machine, it's too difficult to check the code on my phone.

0verEngineer commented 1 year ago

If userB can see the directory structure of userA then there would be a security issue, no discussion on this please, this sould be self explaining.

And you argue that is exacly what userB's app is doing, you argue that the app of userB somehow knows the full path of the lespas root directory and with this info the app is able to decide this is the directory it is interested in.

If this would be true there would be a problem if i just decide i want some lespas-named directories somewhere in my nextcloud

But thats not true, your app does not work that way, i checked it twice.

Step1 is happening in NCShareViewModel.kt Line 256 Step2 is not needed and not done by the app (sorry my fault) you get the name of the json lespas creates by the enpoint called in Step1 from the json field: item_source Step 3 is happening in NCShareViewModel.kt Line 336 (in a loop for each result from Step1) Step 4 is in Lines 336-337

So i have proven two things here:

  1. that that you're answer : Your step 2 and step 3 are exactly what the current implementation try to avoid, which is scanning all the received shared folders is simply wrong, you are scanning all shared folders

  2. Your app does not need the hardcoded directory name lespas for sharing, there is no code where this is cheched or something like that.

I am not able to understand why you don't get this, i do not want to criticize you nor your app, the app is amazing. I just want the app a whole lot user friendly by this.

scubajeff commented 1 year ago

Just took a glance, lespasBase is actually used when getting the list of share by rather than shared with. Sorry for the confusion so far. My memory on this part of the code is really vague, but I'm sure it was a decision about speed back then. We can continute this conversation when I get my hand on my dev machine next week. And yes, get rid of the lespas/ is a good thing.

0verEngineer commented 1 year ago

Very nice, thank you very much

If i have some time and can wrap my head around kotlin android i'll try to create a PR for this issue.

scubajeff commented 1 year ago

PR is always welcome. But I had a feeling that it will take quite sometime to do a though-out testing for this refactoring, since lespasBase is being used 'everywhere'

scubajeff commented 1 year ago

Ok, finally get my hand on my machine. Your observation is right, the lespas/ can be eliminated. It will take a little bit longer to do the refactoring though. I'm working on it now.

scubajeff commented 1 year ago

Had done some tests lately on this issue, it's a surprisingly easy fix so far. But have to figure out a way to make the upgrade works without re-login.

dvalter commented 1 year ago

You can copy or symlink folders to lespas's home folder. The next time you open lespas app, those folders will be synced to your phone as albums.

Maybe I'm missing something, but I could not find the way to symlink a folder into another folder in the way Nextcloud would pick it up. Could you please share the way if you know?

scubajeff commented 1 year ago

@dvalter I'm not sure if you can do that on Nextcloud's webpage, I always do it on command line.

paravoid commented 1 year ago

Had done some tests lately on this issue, it's a surprisingly easy fix so far. But have to figure out a way to make the upgrade works without re-login.

I also would be interested in this! Having to re-login doesn't sound too much of a hassle, so if you'd be willing to release something with the code as -is, I'd be happy to test :)

scubajeff commented 1 year ago

Way had been found to prevent re-login after upgrade. The folder structure also need to incoporate changes from new device gallery backup update, so it's kind of hugh, need more time to smooth it out.

scubajeff commented 1 year ago

release 2.9.0 eliminated the need of the lespas sub-folder name

0verEngineer commented 1 year ago

Very nice, how to migrate if already setup?

scubajeff commented 1 year ago

@0verEngineer please refer to announcement on sub Nextcloud/

0verEngineer commented 1 year ago

What announcement and where?

scubajeff commented 1 year ago

in subreddit https://www.reddit.com/r/NextCloud/comments/14d8ym9/les_pas_290_brand_new_things/

0verEngineer commented 1 year ago

Ahh thanks a lot, however because of the current problems reddit faces it would be good to also have this announcements somewhere else, like in the github release or so.

Thanks a lot for your work :)