feat(mobile): Adds file upload progress stats #7760
Conversation
…ze uploaded, current file size and formatted bytes per second upload speed. Closes immich-app#7379
…R review (just looking around) that localisation is done via Localizely and this was the instruction (to only provide the en-US localisation).
|
This was the reference I found and subsequently why I removed the non-'en-US' translations: https://github.com/immich-app/immich/pull/7323/files/25ca7303acf0bbcfd739d04dc318c6bfb2042867#r1500831992 Hope that was the right thing to do! |
|
Hello, thank you for the PR. I tested and found some bugs below regarding the stats of the upload speed
|
Thanks for further testing the PR! Weird! I didn't have any such issues when testing on mine, but that tale is as old as time when it comes to software dev. Firstly, glad that the file size and uploaded stats appear to work fine. The first two images look like overflows, so I can get some boundary checks added to curtail those. The third one - that's odd. Was it always/consistently displaying '0.0 B/s' or was it just showing it periodically? For example just showing 0 for one second (update period) then correct thereafter? The upload speed is calculated every second to stop calculating on every data state update (as it just looked a bit silly doing it several times per second). |
…counted for on erroneous upload speed calculation, sometimes the numbers received back from the upload handler can be a bit wild.
|
So I've added a boundary check to the value before its stored into the data object to hopefully stop downstream overflow values. However, I'm still unable to re-produce either issue in my local simulator and testing. Is there any re-producible scenario in which the It seems to be working pretty robustly for me, is all. I'd like to fix the bug, but cannot at this time re-produce it locally. Here's an example of an upload for me: Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-03-13.at.21.16.20.mp4 |
…igger overflow issues or 'zero' readouts, left over values from the previous file may do that. So adding the last upload sent bytes to the values to be reset may help! The time isn't necessary, as the period/cycle is inconsequential in this circumstance, well it should be anyway.
…euristic bug fixing. I was thinking it would be advantageous not to reset the update time, as it would trigger a quicker first upload speed calculation. However, I realised that could also cause the calculation to be incorrect on the first cycle as the period wouldn't align. Not really sure if it would be a big deal, but I'm taking wild guesses in the dark here. Again, some purely heuristic debugging as I can't re-produce the underlying issue. This is mainly just ensuring that the state is fully reset and is a known state at the beginning of each file as a common strategy to reduce issues.
|
Just under some purely heuristic debugging (as I can't get a re-producible scenario) I've added some more fixes just to ensure things are reset to a known state at the beginning of each file upload as a common strategy to reduce issues. Not sure if this will be a direct fix, but it will certainly just help keep things to be in an expected state, clean and tidy. |
|
Seems to be a bit more robust now, resetting correctly between cycles/file uploads. Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-03-13.at.21.50.49.mp4 |
|
Thank you! I think I was testing under normal operating conditions, where you can have files get uploaded very quickly. You might be able to reproduce if you only upload images, not videos and put it on 5GHz WIFI |
|
@alextran1502 ah, thanks for the test case! Can confirm, grabbing a load of test images and running an upload, I can see that behaviour now. As the uploads occur so quickly (in under a second), it never has chance to hit the first 'cycle' to calculate the upload speed over the given period. Good thing is the file size progress works spot on, which is the main thing. Although I'm not sure on a 'fix', as the only thing will be to lower the polling rate to sub-one-second to try and squeeze a calculation in before the upload ends. I can understand your push to get the functionality to be consistent, but given its on screen for less than a second anyway, is it a big issue? I'd imagine upload speeds are more desirable to know on longer running transfers. If its done before you can see it then you're kinda winning anyway haha All I can think is to drop the polling rate down, but then I ran into issues with the maths being wonky. The other option is to dynamically hide the upload speed until after the first poll. So items that upload in under a second just display the file size progress without the upload speed. But that may seem like inconsistent UX behaviour. Here's a working re-production of the '0.0 B/s' issue using 10 x 500KB-1.5MB JPEG's (I find setting the player's playback speed to 0.25x helps): Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-03-13.at.22.08.10.mp4 |
|
A potential fix (but it would probably require to change the implementation a lot) would be to compute the upload speed across multiple files, with like an exponential moving average maybe? |
That would be a good way to do it, just persist the upload speed. Hmmm, will have a think on how to do that. It shouldn't be too complex, its mainly just not fully resetting the upload speed, only showing zero when something isn't uploading then restoring the prior value. |
|
@leleogere I was thinking the same thing. If it doesn't work, I am fine to leave the upload speed out, just leave the file's size progress |
|
Not completely in the scope of this PR, but as we gather the number of files to upload in any case, it might be interesting to also get their size, and estimate the remaining time before the end of the upload using the upload speed that is computed. |
… progress bar, it makes more sense there than in the file information table which contains only static information pertaining to the file itself. Switching to a monospace font to keep the UI from jumping around as the numbers change.
…upload speed (as per the discussion on PR immich-app#7760), this stores the 'upload speeds' (capped at the latest 10) in a list and calculates the current upload speed as the average over them. This way the UI can always display a 'constant' upload speed during uploading, instead of starting a fresh when each file starts uploading. Limiting it to the 10 latest keeps the average somewhat recent and ensures some level of sensible memory allocation.
|
Okay, so I've implemented an average upload speed instead of the realtime upload speed and now I see what you guys were wanting as it is much better. I capped it to the latest 10 upload speed calculations, in order to keep things somewhat recent and memory sensitive. I've also done a small tweak to the UI to place the stats directly underneath the progress bar, as its far more relevant being there than it is in the file table. The file table is contextual to the file, so it didn't make sense to have the upload progress stats placed there. Instead the upload progress stats are now in smaller font directly underneath the progress bar, much closer to my initial mockup on #7379. They look far more natural and at home in their new location. I've also swapped to using Immich's monospace font for the stats display, as to ensure that the numbers don't cause the UI to bounce around as the upload stats values change, which looks far better IMO. Its a common pet peeve of mine, and this change makes it far more readable. See what you guys think (@alextran1502 & @leleogere) (apologies for the lag in places, thats iOS simulator for you): Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-03-14.at.18.08.37.mp4 |
|
This feels way better! Nice touch to use a monospace font. |
|
Amazing! I will look at the code later today and merge |
Closes #7379
See screenshot below, specifically its the new "File progress: ..." row, as the new first row in the table at the bottom of the view containing all the current uploading file information.
Adds file upload progress stats:
Tested:
NOTE: This does require localisation for other languages, and I'm happy to be pointed in the right direction for how that's done (e.g. just a Google translate?), as it adds a new language key.This was the reference I found and subsequently why I removed the non-'en-US' translations: https://github.com/immich-app/immich/pull/7323/files/25ca7303acf0bbcfd739d04dc318c6bfb2042867#r1500831992
Hope that was the right thing to do!
Screenshot of the new feature, its the new "File progress: ..." row, as the new first row in the table at the bottom of the view containing all the current uploading file information: