ios-app icon indicating copy to clipboard operation
ios-app copied to clipboard

[feature/last-picked] Add Recent Locations to location picker

Open felix-schwarz opened this issue 7 months ago • 8 comments

Description

Adds a row of recently picked locations to the Location Picker for quick access.

Screenshots (if appropriate):

Within same account Across accounts (media uploads) Share Extension
Bildschirmfoto 2025-05-23 um 12 03 56 Bildschirmfoto 2025-05-23 um 12 07 03 Bildschirmfoto 2025-05-30 um 10 07 49

Types of changes

  • [ ] Bug fix (non-breaking change which fixes an issue)
  • [x] New feature (non-breaking change which adds functionality)
  • [ ] Breaking change (fix or feature that would cause existing functionality to change)

QA

Test plan:

Reports:

  • [X] (1) Move operation shows entries from other spaces https://github.com/owncloud/ios-app/pull/1467#issuecomment-2955428253 [FIXED]
  • [X] (2) Removed folder is retained in the list https://github.com/owncloud/ios-app/pull/1467#issuecomment-2955460826 [WONT FIX]
  • [ ] (3) No padding in cell https://github.com/owncloud/ios-app/pull/1467#issuecomment-2955692861
  • [ ] (4) Cell width expands a lot if display name is longer than usual https://github.com/owncloud/ios-app/pull/1467#issuecomment-2955704542
  • [X] (5) Icons in cells https://github.com/owncloud/ios-app/pull/1467#issuecomment-2955734100 [WONT FIX]

felix-schwarz avatar May 23 '25 10:05 felix-schwarz

Let's QA this one...

jesmrec avatar Jun 09 '25 10:06 jesmrec

(1) [FIXED]

  1. Copy an item in Personal to another space in the same account (location saved)
  2. Select Move of any item in Personal to display the folder picker

Current:

location saved in the step 1. is displayed

Expected:

Since moving items between spaces is not allowed, the locations pointing to other spaces shouldn't be displayed to avoid the consequent error.

Is that feasible?

iPhoneXR v18.4 741040e7

jesmrec avatar Jun 09 '25 10:06 jesmrec

(2) [WONT FIX]

Following the sense of (1):

If a folder that is in the list is removed from the account, it is still in the list of picked locations. Shouldn't be removed as well?

It is correctly handled with a message Folder removed, this is just as an improvement.

NOTE: same for renamed and moved folders

iPhoneXR v18.4 741040e7

jesmrec avatar Jun 09 '25 11:06 jesmrec

(3) [FIXED]

An small glitch, in case the folder name exceeds one line in the recent location cell:

Screenshot 2025-06-09 at 14 41 48

There is no padding in the bottom side of the cell.

iPhoneXR v18.4 741040e7

jesmrec avatar Jun 09 '25 12:06 jesmrec

(4) [FIXED]

Related with (3)

If the display name is longer than expected, the cell width expands till infinite and beyond:

Screenshot 2025-06-09 at 14 45 52

It should be ellipsized, just like the folder name

iPhoneXR v18.4 741040e7

jesmrec avatar Jun 09 '25 12:06 jesmrec

(5) (not blocker) [WONT FIX]

I'm not sure if this is a good UX for the icon in the cell:

Root Non root
Personal Screenshot 2025-06-09 at 14 56 53 Screenshot 2025-06-09 at 14 56 53
Other space Screenshot 2025-06-09 at 14 57 58 Screenshot 2025-06-09 at 14 56 53

I'd set the same icon for any option inside an space (no Personal) and other for Personal.

root/non-root does not seem to be a meaningful differentiation.

What do you think?

iPhoneXR v18.4 741040e7

jesmrec avatar Jun 09 '25 13:06 jesmrec

@jesmrec I fixed (1), (3) and (4) in code:

  • unfit locations are not filtered
  • the number of lines is limited to 1 - and its width limited to 240 points

Regarding (2): that deleted recent locations remain available is a compromise. I had to pick between A) a simple implementation, storing recent locations in the respective bookmark's KVS, quickly accessible and immediately ready for rendering without much overhead B) integration into the SDK and DB, so they are fully tracked (including renames), but more CPU + resource overhead to retrieve them

I went with A to keep the implementation simple. If it should turn out to be a bad or the wrong compromise, it can still be transitioned to B.

Regarding (5): The icon for the Personal space's root folder should also be a "space" icon. If it shows a folder, that should only happen for the Personal folder on OC10, where a folder is also used elsewhere in the product for that folder.

The logic here is to always show the icon belonging to what's represented:

  • the folder "Neuer Ordner" in the "Personal" space is shown with a folder icon and a space icon below it to indicate which space it is located on
  • the root folder of the "Personal" space is shown with a space icon, as it represents the space.

Simulator Screenshot - iPhone 16 Pro - 2025-06-14 at 15 07 29

felix-schwarz avatar Jun 14 '25 13:06 felix-schwarz

(1) -> fixed (2) -> thanks for the clear explanation... let's check how A works, with B as candidate. (3) -> fixed, but pending of (4) (4) not fixed:

Screenshot 2025-06-19 at 09 17 58

(5) -> thanks for the clear explanation

jesmrec avatar Jun 19 '25 07:06 jesmrec

@jesmrec Thanks for the screenshot! Please check if (4) is fixed with the latest commit.

felix-schwarz avatar Jun 23 '25 07:06 felix-schwarz

@jesmrec Thanks for the screenshot! Please check if (4) is fixed with the latest commit.

that's much better @felix-schwarz !

i have another suggestion about the fix. It looks like:

Screenshot 2025-06-23 at 12 31 38

Should't it better to elipsize the long user name as the long folder (with the "..." inline)?

jesmrec avatar Jun 23 '25 10:06 jesmrec

@jesmrec Please try again with the latest commit - that should also use an ellipsis for such long names now.

Edit: Forgot to push the change - so if you tried in the last 10 minutes, please retry.

felix-schwarz avatar Jun 25 '25 08:06 felix-schwarz

@felix-schwarz tested with

app: 499ed8fd sdk: 5399f6b7

are they correct? i don't see the fix. The user's display name overflows and is truncated. Just in case, to be in the same page, this is the long name i expected to be ellipsized:

Screenshot 2025-06-25 at 13 46 33

am i missing something?

jesmrec avatar Jun 25 '25 11:06 jesmrec

@jesmrec Upon further testing and debugging I found conflicting layout instructions that could lead to some ambiguity where it sometimes renders with ellipsis and sometimes they way as seen in your screenshot.

I fixed that with the latest commit. Please check again. It should (finally) be fixed now.

felix-schwarz avatar Jun 27 '25 07:06 felix-schwarz

Now is fixed!

did a second quick round and no regressions happened.

For me, this is OK to go.

jesmrec avatar Jun 27 '25 10:06 jesmrec