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.

Sign up for GitHub

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

Jump to bottom

Communication: Allow users to view and download sent attachments #239

Open
wants to merge 11 commits into
base: develop
Choose a base branch
from
Open

Communication: Allow users to view and download sent attachments #239

wants to merge 11 commits into from

Conversation

julian-wls
Copy link
Contributor

@julian-wls julian-wls commented Dec 18, 2024 •
edited
Loading

Problem Description

As described in #117 all non-image attachments that are sent in the chat are rendered as links and cannot be opened currently due to an authorization error, since there is no authToken attached to the request.

Changes

This PR introduces a Link resolver, that is capable of detecting the target of the link and attaches an authToken if needed. If the link targets a pdf file, the resolver passes the request to a new Pdf View, which is wrapped in a modal bottom sheet. Requests that target other Artemis pages should also be opened in a modal bottom sheet in a web view, which allows attaching cookies, however there were issues (see #245). All other links are now opened as new IntentTab.
The new Pdf view was inspired by this repository. It allows users to share and download the pdf. Pdf can also be shown vertically and horizontally.

Note: The security checks have failed because of the repository being temporarily added to the project. However, it has been removed and the pdf viewer was added manually including various modifications (see pdf package in core:ui module)

Steps for testing

  1. Go into a chat that contains a resource pdf.
  2. Click on it and verify, that the pdf view opens the pdf.
  3. Try downloading, sharing and rotating.
  4. Turn of your internet connection and try loading the pdf again, verify that an appropriate error message is shown.

Screenshots

PdfViewer

@julian-wls julian-wls self-assigned this Dec 19, 2024
@julian-wls julian-wls mentioned this pull request Dec 20, 2024
Open
@julian-wls julian-wls changed the title Communication: Allow users to view sent attachments Communication: Allow users to view and download sent attachments Dec 20, 2024
@julian-wls julian-wls linked an issue Dec 20, 2024 that may be closed by this pull request
Attached files and linked lecture attachments open with authorization error in browser #117
Open
@julian-wls julian-wls marked this pull request as ready for review December 21, 2024 11:03
@julian-wls julian-wls added the ready for review This PR can be reviewed label Dec 21, 2024
FelberMartin
FelberMartin requested changes Dec 21, 2024
View reviewed changes
Copy link
Collaborator

@FelberMartin FelberMartin left a comment

Choose a reason for hiding this comment

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

Looking very good overall, I think this is really a major improvement for the app. Good job! 😊

Besides the code comments, I have a few general remarks:

  1. When opening the pdf, it shows a page count of "Page 3/18", even though only the first 1,5 pages are visible. But I think you mentioned something about the page indicator behaving weirdly, so if there is no easy fix, its also fine if we leave it as is :)
  2. The name of the pdf is currently just the timestamp (if think), could we change this to the name displayed in the markdown link? When I eg download the slides I cant really judge what the content of the file is by its name.
  3. The action labelled with "Rotate PDF" felt quite counter-intuitive for me, from this name I would have expected the pdfs to rotate 90°. But I also can't find a short, better fitting alternative. Its a little bit longer than the other labels, but maybe "Toggle View Mode" would be a better fit, what do you think @julian-wls ?
  4. Somewhere in the code zoom is mentioned, I tried to zoom the pdfs by pinching with two fingers in the bottomSheet, but nothing happened. Is this intended?

image

core/ui/build.gradle.kts Outdated Show resolved Hide resolved
...src/main/kotlin/de/tum/informatics/www1/artemis/native_app/core/ui/compose/ArtemisPdfView.kt Show resolved Hide resolved
...ormatics/www1/artemis/native_app/core/ui/markdown/link_resolving/MarkdownLinkResolverImpl.kt Outdated Show resolved Hide resolved
...ormatics/www1/artemis/native_app/core/ui/markdown/link_resolving/MarkdownLinkResolverImpl.kt
val context = LocalContext.current

val (bottomSheetLink, setLinkToShow) = remember { mutableStateOf<String?>(null) }
val (bottomSheetState, setBottomSheetState) = remember { mutableStateOf(LinkBottomSheetState.WEBVIEWSTATE) }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just wondering, is there a particular reason why you used the syntax of val (bottomSheetLink, setLinkToShow) = remember { mutableStateOf<String?>(null) } instead of val bottomSheetLink by remember { mutableStateOf<String?>(null) } here?

Copy link
Contributor Author

@julian-wls julian-wls Dec 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The main reason was, that I had a hard time debugging it at some point, cause the code was not quite readable before. That's the reason I changed it to this syntax.

Edit: I think toggle view mode is quite long and makes the menu dialog quite big, what do you think:
addddd

...ormatics/www1/artemis/native_app/core/ui/markdown/link_resolving/MarkdownLinkResolverImpl.kt Outdated Show resolved Hide resolved
@julian-wls
Copy link
Contributor Author

  • When opening the pdf, it shows a page count of "Page 3/18", even though only the first 1,5 pages are visible. But I think you mentioned something about the page indicator behaving weirdly, so if there is no easy fix, its also fine if we leave it as is :)
  • The name of the pdf is currently just the timestamp (if think), could we change this to the name displayed in the markdown link? When I eg download the slides I cant really judge what the content of the file is by its name.
  • The action labelled with "Rotate PDF" felt quite counter-intuitive for me, from this name I would have expected the pdfs to rotate 90°. But I also can't find a short, better fitting alternative. Its a little bit longer than the other labels, but maybe "Toggle View Mode" would be a better fit, what do you think @julian-wls ?
  • Somewhere in the code zoom is mentioned, I tried to zoom the pdfs by pinching with two fingers in the bottomSheet, but nothing happened. Is this intended?

Thanks for reviewing. I really appreciate the feedback :)

Here are the answers:

  1. It actually shows three pages when you open the entire modal bottom sheet. There is a function in VerticalPdfReaderState, which determines how many pages are shown and sets the current page to the last page that is currently open. If we change this the last page of the pdf would never be reached.
  2. I thought about this too, but I didn't find any way to access the string in the markdown text (I think that would be the only way to get the name). The Link resolver only checks when a link has been clicked and only returns the link, not the title.
  3. I think you're right. Toggle view mode sounds good to me. Would you still keep this icon, though? @FelberMartin
  4. For zooming you have to double tap the page, first.

@eylulnc
Copy link
Contributor

eylulnc commented Dec 22, 2024 •
edited
Loading

Thanks for the great work! This is indeed a huge improvement 😊

I checked it out based on your previous discussions and have a few observations:
Page Indicator: Regarding the ‘Page 3/18’ indicator, I didn’t encounter this issue during my testing—it seems to be working fine for me.
Bottom Sheet Behavior: I really like the toast—it’s a nice touch! However, I’d prefer not to see an empty bottom sheet when there’s an error (e.g., no internet connection), as it feels a bit disconnected.

Screenshot 2024-12-22 at 15 58 38

Naming: I agree it would be nice to have more meaningful names for downloaded files. However, if this isn’t feasible, it’s fine as is.

Screenshot 2024-12-22 at 16 00 05

Rotation Action: Initially, I found the label “Rotate PDF” a bit confusing because I expected it to change the PDF’s orientation. Since it toggles between scroll and swipe modes, maybe a label like “Read Mode” could help clarify this. Keeping the current icon is fine, but if you feel the label doesn’t cause confusion, I think it’s okay to keep it as is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FelberMartin FelberMartin FelberMartin requested changes

@eylulnc eylulnc Awaiting requested review from eylulnc

Requested changes must be addressed to merge this pull request.

Assignees

@julian-wls julian-wls

Labels
ready for review This PR can be reviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Attached files and linked lecture attachments open with authorization error in browser
3 participants
@julian-wls @eylulnc @FelberMartin