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

fix: Use correct unit for file sizes and fix user quota #40621

Closed
wants to merge 3 commits into from

Conversation

susnux
Copy link
Contributor

@susnux susnux commented Sep 25, 2023

Summary

We always used binary file sizes (base 2, meaning 2048 bytes = 2KiB), but we added the wrong unit (we used the decimal (base 10) unit e.g. 2KB).

This fixes the user management showing a wrong quota (#38874 for master)

Also the OC_Helper now supports base 10 and base 2 in computerFileSize, for backwards compatibility it defaults to force even units like KB to be binary.

Screenshots

Screenshot comment
Screenshot_20230925_163450 As discussed during the Nextcloud conf, we always expect the user to mean the binary size (mostly they are confused by windows using the wrong unit)
Screenshot 2023-09-25 at 16-33-31 Aktive Benutzer - Benutzer - Benutzer - Nextcloud Quota is now displayed correctly
Screenshot 2023-09-25 at 16-32-58 Dateien - Nextcloud The file list automatically adapted to the correct unit

Checklist

@susnux susnux force-pushed the fix/wrong-file-size-unit-used branch 4 times, most recently from 0ba7a5a to 7c4b489 Compare September 26, 2023 11:23
@susnux susnux force-pushed the fix/wrong-file-size-unit-used branch 4 times, most recently from fdb40e5 to b145032 Compare October 9, 2023 10:43
We always used binary file sizes (base 2, meaning 2048 bytes = 2KiB),
but we added the wrong unit (we used the decimal (base 10) unit e.g. 2KB).

This fixes the user management showing a wrong quota.

Also the OC_Helper now supports base 10 and base 2 in `computerFileSize`,
for backwards compatibility it defaults to force even units like `KB` to be binary.

Signed-off-by: Ferdinand Thiessen <[email protected]>
Signed-off-by: Ferdinand Thiessen <[email protected]>
@susnux susnux force-pushed the fix/wrong-file-size-unit-used branch from b145032 to aad8513 Compare October 11, 2023 13:13
@susnux
Copy link
Contributor Author

susnux commented Oct 17, 2023

cc @sorbaugh and @AndyScherzinger

The problem:

  • On Nextcloud up to (including) 26 we used decimal units like 10 KB or 20 GB but we always meant 10 KB = 10 * 1024 bytes and not 10 KB = 10 * 1000 bytes.
  • On Nextcloud 27 there was a regression (@nextcloud/files) that switched to even decimal values, meaning 10KB = 10 * 1000 bytes but our backend was and is always using binary values (1024).
  • On Nextcloud 27.1 we patched this and reverted back to binary values (10* 1024) but with decimal units (KB instead of the proper KiB).

But for current master the frontend is still broken, we have to options:

  1. Everything stays like it was on Nextcloud 26 and below, this requires the @nextcloud/files library to be adjusted again.
  2. Move to decimal values (we should not do this the only people doing this are hard drive manufacturers and Apple)
  3. Show the proper units (KiB instead of KB, GiB instead of GB...) this is what this PR is about.

From my point of view only 1 and 3 are really considerable solutions.
Pro 1:

  • Everything stays the same UI wise
  • Windows is using the wrong units for decades now, so the majority of users are used to them
    Con 1:
  • See the confusion in the linked issue, having one unit used for multiple different things confuses

Pro 3 (this PR):

  • It is clear what unit and file sizes to expect, so less confusion
    Con 3:
  • Non technical users might choke on the i in the unit (but probably it will even not attract attention)
  • Changing UI

@sorbaugh
Copy link
Contributor

While it's true that lots of users are conditioned to read KB, GB, etc, I think switching to the correct units would be a "soft enough" change UX wise so I would vote for option 3 (but would be fine with option 1).

@jancborchardt
Copy link
Member

I’d say it’s best in this case to go with what Windows does since @susnux as you say that is what the majority of people are used to. Yes it is not correct, but it is sort of convention on a topic where many people are confused anyway.

@sorbaugh
Copy link
Contributor

I don't have a strong opinion on this, so option 1 is fine as well if we can foresee that option 3 would be confusing to people.

@skjnldsv skjnldsv mentioned this pull request Nov 1, 2023
This was referenced Nov 6, 2023
This was referenced Nov 14, 2023
@blizzz blizzz modified the milestones: Nextcloud 28, Nextcloud 29 Nov 23, 2023
@skjnldsv skjnldsv added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Feb 23, 2024
This was referenced Mar 12, 2024
This was referenced Mar 20, 2024
@skjnldsv skjnldsv mentioned this pull request Mar 28, 2024
81 tasks
@skjnldsv skjnldsv modified the milestones: Nextcloud 29, Nextcloud 30 Mar 28, 2024
This was referenced Jul 30, 2024
This was referenced Aug 5, 2024
@skjnldsv skjnldsv mentioned this pull request Aug 13, 2024
@skjnldsv skjnldsv removed this from the Nextcloud 30 milestone Aug 14, 2024
@susnux
Copy link
Contributor Author

susnux commented Aug 27, 2024

It was decided to keep current style.

@susnux susnux closed this Aug 27, 2024
@susnux susnux deleted the fix/wrong-file-size-unit-used branch August 27, 2024 21:20
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.

6 participants