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

[FEATURE REQUEST] Open and create .url shortcut files #4420

Merged
merged 20 commits into from
Jun 20, 2024

Conversation

Aitorbp
Copy link
Collaborator

@Aitorbp Aitorbp commented Jun 10, 2024

Related Issues

App: #4413

  • Add changelog files for the fixed issues in folder changelog/unreleased. More info here
  • Add feature to Release Notes in ReleaseNotesViewModel.kt creating a new ReleaseNote() with String resources (if required)

QA

Test plan: https://github.com/owncloud/QA/blob/master/Mobile/Android/Executions/Release_4.3/Shortcuts.md

Reports and improvements:

@Aitorbp Aitorbp self-assigned this Jun 10, 2024
@Aitorbp Aitorbp linked an issue Jun 10, 2024 that may be closed by this pull request
12 tasks
@Aitorbp Aitorbp force-pushed the feature/open_create_url_shortcut_files branch 2 times, most recently from ba59abb to 3567b11 Compare June 11, 2024 07:35
Copy link
Collaborator

@JuancaG05 JuancaG05 left a comment

Choose a reason for hiding this comment

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

Some questions and changes requested here @Aitorbp!

@Aitorbp Aitorbp force-pushed the feature/open_create_url_shortcut_files branch from f80f80d to bcb571c Compare June 13, 2024 09:00
@Aitorbp Aitorbp requested a review from JuancaG05 June 13, 2024 09:20
Copy link
Collaborator

@JuancaG05 JuancaG05 left a comment

Choose a reason for hiding this comment

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

Some more changes here @Aitorbp. Pay attention to the comments in previous CR, you skipped some of them

Copy link
Collaborator

@JuancaG05 JuancaG05 left a comment

Choose a reason for hiding this comment

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

LGTM now!

@jesmrec
Copy link
Collaborator

jesmrec commented Jun 14, 2024

(1) [FIXED]

This is our current implementation together with iOS and web:

Android iOS Web
Screenshot 2024-06-14 at 12 39 14 Screenshot 2024-06-14 at 12 36 03 Screenshot 2024-06-14 at 12 36 25
  • iOS and web put the URL field above, and the shortcut name below, just the opposite as we do. We should align behaviour across platforms.

  • iOS and web call the submit button as Create or Create shortcut, whereas we call it YES (i guess, this is an Android default). We should go for Create as well.

  • I'd remove the word new from the title. If you create something, this is always going to be new.

Pixel 2, Android11
87d3a1d2

@jesmrec
Copy link
Collaborator

jesmrec commented Jun 14, 2024

(2) [IMPROVEMENT] [DONE]

As improvement, i'd be cool to have the file name like in web:

Screenshot 2024-06-14 at 12 44 57

the .url at the end makes clear that the file name is the requested field, and the extension must not be typed by the user.

Pixel 2, Android11
87d3a1d2

@jesmrec
Copy link
Collaborator

jesmrec commented Jun 14, 2024

(3) [FIXED]

Tiny detail for long names:

error message is: File name must not be longer than 223 characters

By typing exactly 223 characters, the message is there. It shouldn't or changing the value to 222

Pixel 2, Android11
87d3a1d2

@jesmrec
Copy link
Collaborator

jesmrec commented Jun 14, 2024

(4) [FIXED]

About this comment:

I format it by default with http because if the websites do not support https, the websites will say: The site can't be reached. For example this one: http://www.edu4java.com/es/web/web30.html.
On the contrary, if we write a website with http, but it supports https, it will redirect us directly to https.

but, what happens if it supports http and not https? you'll get the error as well. You can check this with a basic oC10 server. The fact of supporting http, https or both is a matter of the hosting server, each one has its own setup and we can not assume an specific behaviour based in only one example.

Both web and iOS use https as default. I'd go for this.

Pixel 2, Android11
87d3a1d2

@jesmrec
Copy link
Collaborator

jesmrec commented Jun 14, 2024

(5) [FIXED]

Create a shortcut and submit it

Current:

In the uploads view, the entry does not have a proper icon:

Screenshot 2024-06-14 at 13 51 09

Expected:

Could be used the same icon as in the list of files?

Pixel 2, Android11
87d3a1d2

@jesmrec
Copy link
Collaborator

jesmrec commented Jun 14, 2024

(6) [FIXED]

I'm not pretty sure to ellipsize the URL when it is very long. The URL to open is sensible information that is hidden to the user. Let me know if i'm wrong, but it is being ellipsized if the length is longer than 1 line. That means, almost every URL that points to a subfolder/subdomain is going to be ellipsized.

I'd remove that, or expand to a longer size

Pixel 2, Android11
87d3a1d2

@Aitorbp
Copy link
Collaborator Author

Aitorbp commented Jun 17, 2024

(6)

I'm not pretty sure to ellipsize the URL when it is very long. The URL to open is sensible information that is hidden to the user. Let me know if i'm wrong, but it is being ellipsized if the length is longer than 1 line. That means, almost every URL that points to a subfolder/subdomain is going to be ellipsized.

I'd remove that, or expand to a longer size

Pixel 2, Android11 87d3a1d2

Regarding the ellipsize of the URL when it is very long, the following occurs: if we want to put a maximum of three lines and set a truncated ellipsize in the middle, it will not work. Android does not have this functionality automatically, it would have to be programmed. Therefore the following options are considered:

  • Set the ellipsize at the end of the text, keeping a maximum of three lines.
  • Set the ellipsize in the middle with a maximum of three lines, making a method programmatically. A calculation would have to be made to put the ellipsize in the center of the second line. I don't know how this would work...
  • Set a HorizontalScrollLayout so that it scrolls if the text exceeds the maximum of one line. With this component it only detects one line, I understand that it could also be done programmatically so that it detects more lines.
  • The last option would be to leave it as iOS has it, without any type of restriction and that the URL occupies all it needs.

@jesmrec @JuancaG05

@JuancaG05
Copy link
Collaborator

  • Set a HorizontalScrollLayout so that it scrolls if the text exceeds the maximum of one line. With this component it only detects one line, I understand that it could also be done programmatically so that it detects more lines.

I would discard this option directly. A scroll in a dialog is no good practice.

  • The last option would be to leave it as iOS has it, without any type of restriction and that the URL occupies all it needs.

This is not a good option for me. What if the URL is so so long (not strange case currently) that the dialog gets too big and we cannot see its buttons? We need to set a limit somewhere

Why is it that you can ellipsize at the end a text of more than 1 line but not in the middle?

@jesmrec
Copy link
Collaborator

jesmrec commented Jun 17, 2024

This is not a good option for me. What if the URL is so so long (not strange case currently) that the dialog gets too big and we cannot see its buttons? We need to set a limit somewhere

Does not the dialog (including the buttons) expand? i mean, if the text is too long, the buttons are on the bottom in the same way, just the central is expanded.

@JuancaG05
Copy link
Collaborator

Does not the dialog (including the buttons) expand? i mean, if the text is too long, the buttons are on the bottom in the same way, just the central is expanded.

Yes, the center will expand, but... what if it expands so much that the buttons get out of the screen?

@Aitorbp
Copy link
Collaborator Author

Aitorbp commented Jun 18, 2024

To sum up, to align with iOS behavior, we're going to leave the url unrestricted to the maximum number of lines, but add a scrollview so that when the text takes up more space than the screen, the url element will scroll.

@Aitorbp Aitorbp force-pushed the feature/open_create_url_shortcut_files branch from f353ed7 to 3b28f67 Compare June 18, 2024 09:36
@Aitorbp
Copy link
Collaborator Author

Aitorbp commented Jun 18, 2024

All reports are ready to be tested again @jesmrec

@jesmrec
Copy link
Collaborator

jesmrec commented Jun 18, 2024

everything is fixed. Now, i will face a 2nd QA round.

@jesmrec
Copy link
Collaborator

jesmrec commented Jun 18, 2024

(7) [FIXED]

Minor change

Typing a URL with blanks, the error The URL cannot contain spaces is displayed.

This is correct, but i'd use blanks instead of spaces, since "spaces" has a different meaning... it may lead to confusion

@Aitorbp
Copy link
Collaborator Author

Aitorbp commented Jun 18, 2024

Changed report (7), ready to test @jesmrec

@jesmrec
Copy link
Collaborator

jesmrec commented Jun 19, 2024

Related owncloud/web#11063

I noticed that the shorcuts created in the web client (oCIS) are not open in mobile clients if they lack of protocol prefix. If i'm not wrong, the current approach in Android app is adding the default protocol prefix (https) when the shortcut is open (not in creation), right @Aitorbp ?

So, we have two options here:

  • Getting the current approach as correct for the moment and going ahead. If the prefix default protocol should be added in creation phase as before, it could be changed in a new PR

  • Wait till a solution across the clients is agreed in the web issue mentioned above.

which one do you like the most?

@Aitorbp
Copy link
Collaborator Author

Aitorbp commented Jun 19, 2024

Related owncloud/web#11063

I noticed that the shorcuts created in the web client (oCIS) are not open in mobile clients if they lack of protocol prefix. If i'm not wrong, the current approach in Android app is adding the default protocol prefix (https) when the shortcut is open (not in creation), right @Aitorbp ?

So, we have two options here:

  • Getting the current approach as correct for the moment and going ahead. If the prefix default protocol should be added in creation phase as before, it could be changed in a new PR
  • Wait till a solution across the clients is agreed in the web issue mentioned above.

which one do you like the most?

Right we add the default protocol prefix when the shortcut is open. In my opinion, I would go for the current approach, even if the web were to set the protocol, the application would not be affected.

@jesmrec
Copy link
Collaborator

jesmrec commented Jun 19, 2024

@Aitorbp
Copy link
Collaborator Author

Aitorbp commented Jun 20, 2024

Added back the protocol in shortcut creation.

Aitorbp and others added 19 commits June 20, 2024 08:33
…esButton func and changed naming isCreateShortcutButtonEnabled
… change extractUrlFromFile to the class and minor fixing
@Aitorbp Aitorbp force-pushed the feature/open_create_url_shortcut_files branch from bcbaede to d36ccf3 Compare June 20, 2024 07:33
@jesmrec
Copy link
Collaborator

jesmrec commented Jun 20, 2024

That's fixed, now adding the protocol in both creation and opening.

@jesmrec
Copy link
Collaborator

jesmrec commented Jun 20, 2024

Approved, ready to go! 💯

@Aitorbp Aitorbp merged commit 4bffb3a into master Jun 20, 2024
5 checks passed
@Aitorbp Aitorbp deleted the feature/open_create_url_shortcut_files branch June 20, 2024 08:29
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.

[FEATURE REQUEST] Open and create .url shortcut files
3 participants