icloud-photos-downloader / icloud_photos_downloader

A command-line tool to download photos from iCloud
MIT License
7.02k stars 563 forks source link

feat: Update web ui and more #916

Closed holomekc closed 4 months ago

holomekc commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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

holomekc commented 4 months ago

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