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

feat: Update web ui and more #916

Merged
merged 1 commit into from
Jul 23, 2024

Conversation

holomekc
Copy link
Contributor

  • Use bootstrap as component framework and replace all templates with styled elements
  • Add htmx response-targets for error handling
  • Add custom js to open bootstrap toast
  • Add cancel and resume feature

It was a bit quick, but maybe you can guide me a little bit, regarding what you do not like and what is ok.
Here how it looks like:

image
image
image
image
image

@AndreyNikiforov
Copy link
Collaborator

Thanks for taking initiative and participating in making icloudpd better!

It was a bit quick, but maybe you can guide me a little bit, regarding what you do not like and what is ok.

WebUI was added to support headless installs (entering password and MFA is core functionality). The first version of WebUI is very crude, both visually and architecturally. My path forward is making WebUI architecturally coherent with the rest of the app, have solid protection of core functionality against regressions (e.g. static typing, automated testing), and then dive into long requested features and bugs with higher confidence of not breaking existing behavior.

Having WebUI improving [visually] in parallel would be great. Reliability, stability, and compatibility of the web interface I value significantly higher than visual appeal considering use case (entering pass/mfa once a month). I tried to keep js to bare minimum with htmx. Some simple css styling framework seems reasonable as well.

If your expertise is in web ui world, some advise/ideas would be welcome for using web notification for WebUI (so that phone shows local notification when password/mfa is needed). see comment

@holomekc
Copy link
Contributor Author

Hi @AndreyNikiforov ,

The for the quick feedback. I will first check what I forgot or messed up. I have some difficulties to execute the tests etc. on my mac. I think I can manage though.

Regarding notifications. I will check if I can do something about that.

I think I like what you mentioned regarding minimalistic etc. I am not so sure regarding the message exchange between the backend and frontend. It feels like both are writing to dtos and maybe the relevant information is available. I think a more event driven approach would be better. This would include websockets for example. I saw that Flask and htmx supports that. I did not check yet how this is used in the backend though. I did not want to apply to big changes.

I am also not so happy with the additional config and progress class.
Although, I think the config could be a dedicated class. There are so many different configs and it would make the extention of those easier.

@AndreyNikiforov
Copy link
Collaborator

The for the quick feedback. I will first check what I forgot or messed up. I have some difficulties to execute the tests etc. on my mac. I think I can manage though.

docker with dev container on mac should run tests, lint, type_check without problems (and without a need to install python on the host). I am also using GitHub codespaces with the same success. You need to get lint, test, type_check to green before I merge.

Regarding notifications. I will check if I can do something about that.

I suspect it may be a big research. I suggest time boxing the effort.

I think I like what you mentioned regarding minimalistic etc. I am not so sure regarding the message exchange between the backend and frontend. It feels like both are writing to dtos and maybe the relevant information is available. I think a more event driven approach would be better. This would include websockets for example. I saw that Flask and htmx supports that. I did not check yet how this is used in the backend though. I did not want to apply to big changes.

The first thing I would like to address is the representation of the processing state within the app and matching it to necessary actions (==state machinery). Because state transitions are complex (e.g. re-auth in the middle of transfer?) that complexity needs to be easily maintainable and reliable. I need to try that to see how it ends up in python.

Once I have that, then the interface to WebUI will follow the pattern. Re websocket - I wanted a simple pull from the client so it is easier to debug and support, especially at the beginning when I planned to do refactoring of the backend and be-fe comm. If you see value switching, I am open to discussing that (better in a separate gh issue).

I am also not so happy with the additional config and progress class. Although, I think the config could be a dedicated class. There are so many different configs and it would make the extention of those easier.

Config on the app became complex over the years. I prioritize WebUI on 1) receiving input 2) showing current state/status 3) configuration (maybe not just showing, but even entering/editing). I think that for most users who setup icloudpd and figure out parameter set they need, the value of duplicating these params in ui will be minimal. Users who need UI to enter config (as opposite to cli) is a different group and we are not serving them (yet); and for them entering/editing config may have value.

Re Progress: One way is to model it as part of the state transitions, e.g. DOWNLOADING(123, 789) - a state of downloading where 123 bytes already transferred out of 789.

@holomekc
Copy link
Contributor Author

Hi @AndreyNikiforov ,

I hope I covered everything now. Thx for the help.

Regarding notifications. I think it is not possible without a server, which holds the private key, and I guess this is where all just local solutions then fail to establish that. You cannot put the private key in the app. I mean it is most likely not that much effort to establish that in AWS, but this will create some coasts:
https://developer.apple.com/documentation/usernotifications/sending-web-push-notifications-in-web-apps-and-browsers#Enable-push-notifications-for-your-webpage-or-web-app

If the user grants permission, register the provided push notification endpoint and encryption keys from the subscription on your push server with the user’s account.

@holomekc
Copy link
Contributor Author

Sorry @AndreyNikiforov,
I will quickly fix some tiny mistakes in the index.html regarding import and html tags. Should take a few mins.

- Use bootstrap as component framework
  and replace all templates with styled
  elements
- Add htmx response-targets for error handling
- Add custom js to open bootstrap toast
- Add cancel and resume feature
- Fix lint errors
- Fix counter
- Fix typings
- Fix formatting
@holomekc
Copy link
Contributor Author

holomekc commented Jul 23, 2024

Ok done. I also added the manifest.json. This makes it a little bit more awesome to add it as a web app.

@AndreyNikiforov AndreyNikiforov merged commit 09e2811 into icloud-photos-downloader:master Jul 23, 2024
377 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants