Skip to content

[stable27] Revert change that made OC.Util.humanFileSize return decimal sizes instead of binary - #40605

Merged
susnux merged 2 commits into
stable27from
fix/format-file-sizes
Sep 25, 2023
Merged

[stable27] Revert change that made OC.Util.humanFileSize return decimal sizes instead of binary#40605
susnux merged 2 commits into
stable27from
fix/format-file-sizes

Conversation

@susnux

Copy link
Copy Markdown
Contributor

Summary

Previously the OC.Util.humanFileSize was returning file sizes base 2, meaning 1024 bytes = 1 KiB, but the @nextcloud/files library had a regression that set the default to base 10. Meaning 1000 bytes = 1 KB.

This is fixed for current @nextcloud/libraries (3.0.0-beta.23) but for stable27 we need to fix this manually by wrapping the function.

Checklist

@susnux
susnux requested review from a team, Pytal, artonge, nfebe and szaimen and removed request for a teamSeptember 24, 2023 14:54
@susnuxsusnux added this to the Nextcloud 27.1.2 milestone Sep 24, 2023
@susnux
susnuxforce-pushed the fix/format-file-sizes branch from 4539c7b to 44ca6bfCompareSeptember 24, 2023 14:59
…nstead of base 2
Previously the `OC.Util.humanFileSize` was returning file sizes base 2, meaning 1024 bytes = 1 KiB, but the `@nextcloud/files` library had a regression that set the default to base 10. Meaning 1000 bytes = 1 KB.
This is fixed for current `@nextcloud/libraries` but for stable27 we need to fix this manually by wrapping the function.
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@susnux
susnuxforce-pushed the fix/format-file-sizes branch from 44ca6bf to b7a939dCompareSeptember 24, 2023 15:13
@susnux

Copy link
Copy Markdown
ContributorAuthor

PS: This is just a bugfix so I will not touch the issue that we using binary sizes but showing decimal units. This just restores the previous behavior.

@susnuxsusnux changed the title fix: Revert change that made OC.Util.humanFileSize return decimal sizes instead of binary[stable27] Revert change that made OC.Util.humanFileSize return decimal sizes instead of binarySep 24, 2023
@susnux

Copy link
Copy Markdown
ContributorAuthor

drone unrelated

@susnuxsusnux linked an issue Sep 25, 2023 that may be closed by this pull request
8 tasks
@susnux
susnux merged commit 049aa5d into stable27Sep 25, 2023
@susnux
susnux deleted the fix/format-file-sizes branch September 25, 2023 13:27
@come-nc

Copy link
Copy Markdown
Contributor

I think this broke node tests: https://github.com/nextcloud/server/actions/runs/6301358504/job/17107304772

 OCA.Files.FileList tests
Removing files from the list
✗ Removes file from list when calling remove() and updates summary
Expected '69 KB' to equal '70 KB'.
at Object.<anonymous> (apps/files/tests/js/filelistSpec.js:542:46)
Deleting files
✗ calls delete.php, removes the deleted entries and updates summary
Expected '57 KB' to equal '58 KB'.
at apps/files/tests/js/filelistSpec.js:612:47
List rendering
✗ updates summary using the file sizes
Expected '69 KB' to equal '70 KB'.
at Object.<anonymous> (apps/files/tests/js/filelistSpec.js:1141:46)
OCA.Files.FileSummary tests
✗ renders summary as text
Expected '250 KB' to equal '256 KB'.
at Object.<anonymous> (apps/files/tests/js/filesummarySpec.js:47:47)
✗ increases summary when adding files
Expected '500 KB' to equal '512 KB'.
at Object.<anonymous> (apps/files/tests/js/filesummarySpec.js:71:47)
✗ decreases summary when removing files
Expected '125 KB' to equal '128 KB'.
at Object.<anonymous> (apps/files/tests/js/filesummarySpec.js:89:47)
✗ renders filtered summary as text
Expected '250 KB' to equal '256 KB'.
at Object.<anonymous> (apps/files/tests/js/filesummarySpec.js:107:47)
✗ increases filtered summary when adding files
Expected '500 KB' to equal '512 KB'.
at Object.<anonymous> (apps/files/tests/js/filesummarySpec.js:136:47)
✗ decreases filtered summary when removing files
Expected '125 KB' to equal '128 KB'.
at Object.<anonymous> (apps/files/tests/js/filesummarySpec.js:158:47)

@susnuxsusnux mentioned this pull request Sep 26, 2023
4 tasks
@blizzzblizzz mentioned this pull request Sep 26, 2023
@luzfcb

Copy link
Copy Markdown

@susnux@artonge Do you have any idea if the problem that this PR fixes could possibly also be the cause of nextcloud/android#11974 ?

@come-nc

Copy link
Copy Markdown
Contributor

@susnux@artonge Do you have any idea if the problem that this PR fixes could possibly also be the cause of nextcloud/android#11974 ?

No, this PR only touches the web UI

@Jerome-Herbinet

Jerome-Herbinet commented Oct 11, 2023

Copy link
Copy Markdown
Member

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

Labels

3. to reviewWaiting for reviewsbugregression

Projects

None yet

Development

Successfully merging this pull request may close these issues.

wrong user quota in the user administration

5 participants

@susnux@come-nc@luzfcb@Jerome-Herbinet@artonge