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

Setting for number format in archive tab #1719

Merged
merged 17 commits into from
Aug 11, 2023

Conversation

bigtedde
Copy link
Collaborator

@bigtedde bigtedde commented May 23, 2023

Description

Fixes #1713

I am introducing a setting in the Misc tab that allows the user to decide if they want archive sizes in the archive tab to be displayed in a fixed or dynamic number format.

Right now this change adds a setting in the Misc tab, and when the setting is changed it emits a pyqt signal to call populate_from_profile in the ArchiveTab, which checks the setting and displays the archive size in a fixed or dynamic number format accordingly. Here is a short demo:

ezgif-4-28cd7ce8b4

Related Issue

#1595
#1598
#1692

Motivation and Context

#1713

How Has This Been Tested?

Added unit test to ensure the setting updates the archive table units as expected. Test locally, and passes all GitHub CI checks.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have read the CONTRIBUTING guide.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

I provide my contribution under the terms of the license of this repository and I affirm the Developer Certificate of Origin.

@real-yfprojects
Copy link
Collaborator

I added the Fixes keyword so that the issue will be closed automatically when merging the PR.

@bigtedde
Copy link
Collaborator Author

I added the Fixes keyword so that the issue will be closed automatically when merging the PR.

Thank you.

I changed the typo in setting tooltip, and have begun work writing tests.

@bigtedde bigtedde marked this pull request as draft May 25, 2023 00:18
@hariseldon78
Copy link
Contributor

Hey, I'm the one who made the single unit pr in the first place, sorry for replying so late.

I'm glad you jumped in and made the setting yourself, it's a good idea. I also propose to improve my algorithm so if someone want to have a single size unit, and there are big differences like in your case, the smallest sizes could be rounded to zero, maybe with a symbol that indicates near 0.

It's probably difficult to come up with a configuration that satisfy everybody, so let's talk about it and if we find a good fit i can do the change myself.

@bigtedde
Copy link
Collaborator Author

I also propose to improve my algorithm so if someone want to have a single size unit, and there are big differences like in your case, the smallest sizes could be rounded to zero, maybe with a symbol that indicates near 0.

I really like your idea here. The question that comes to my mind is what the threshold would be for something to be considered 'near 0' and what approach to take to implement this?

@bigtedde
Copy link
Collaborator Author

I added the tests for the archive units setting, pretty_bytes_dynamic_units, and the pyqt signal that connects the setting to a refresh of the archive tab.

Additionally, I renamed pretty_bytes to pretty_bytes_fixed_units to help make it distinct from pretty_bytes_dynamic_units

@real-yfprojects @m3nu I am eager to hear your thoughts, and what I can do to get this ready to review for merge.

@m3nu
Copy link
Contributor

m3nu commented May 26, 2023

Looks good when quickly looking over it.

I do think the challenge here is more human than technical. So let's also ping @hariseldon78 who first added the feature in Feb.

@real-yfprojects
Copy link
Collaborator

So let's also ping @hariseldon78 who first added the feature in Feb.

He is already part of the discussion: #1719 (comment)

@bigtedde
Copy link
Collaborator Author

bigtedde commented May 26, 2023

@hariseldon78 FWIW I left your code largely unchanged, with only a rename of the pretty_bytes method. My aim with this PR was narrow - add back the old pretty_bytes method from v8.10 and offer users a setting to decided which format is best for them. I am eager to hear your thoughts!

@hariseldon78
Copy link
Contributor

FWIW I approve completely adding a setting, I always appreciate configurability in a software. I'm not sure which methods should be set as default at installation, I leave that to your sensibility.

As for the improvement of my algorithm at the moment I don't have the possibility of putting much time in it, but if anybody have ideas on the matter I'm happy to discuss it here or on another issue, just ping me!

Looking at my archive list at the moment it is shown in MB, just because 2 on 20 archives are smaller than 100 MB... All the others are at least near a GB or in one case (the first backup) as big as 150 GB, making it hard, at a glance, to see if we are talking 1.5 TB or 150 GB. So it's definitely not perfect, maybe adding the thousands separation could help, but then you need to deal with localization formats because everyone have different ideas about how big numbers should be written...

@real-yfprojects
Copy link
Collaborator

maybe adding the thousands separation could help, but then you need to deal with localization formats because everyone have different ideas about how big numbers should be written

Qt might provide methods for that. However I would prefer discussing this in a separate issue.

@bigtedde bigtedde marked this pull request as ready for review May 29, 2023 19:29
@bigtedde bigtedde changed the title Setting for new number format (work in progress) Setting for number format in archive tab May 29, 2023
src/vorta/utils.py Outdated Show resolved Hide resolved
src/vorta/utils.py Outdated Show resolved Hide resolved
src/vorta/utils.py Outdated Show resolved Hide resolved
tests/test_utils.py Outdated Show resolved Hide resolved
src/vorta/views/archive_tab.py Outdated Show resolved Hide resolved
@bigtedde
Copy link
Collaborator Author

bigtedde commented Aug 1, 2023

@real-yfprojects Thank you for the feedback on this setting. I have addressed each of your concerns with the most recent commits, tested the setting locally, and everything appears to work as intended.

Comment on lines 269 to 282
def pretty_bytes_dynamic_units(size, metric=True, sign=False, precision=1):
if not isinstance(size, int):
return ''
prefix = '+' if sign and size > 0 else ''
power, units = (10**3, METRIC_UNITS) if metric else (2**10, NONMETRIC_UNITS)
n = find_best_unit_for_size(size, metric=metric, precision=precision)
size /= power**n
try:
unit = units[n]
return f'{prefix}{round(size, precision)} {unit}B'
except KeyError as e:
logger.error(e)
return "NaN"

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that you changed this method to use find_best_unit_for_size it does exactly the same as pretty_bytest_fixed_units when passing fixed_unit=None. Consequently we only need the latter.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just combined these two into one pretty_bytes() function, and rewrote the tests to ensure functionality.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, went through the files affected by previous (and now reverted) pretty_bytes name change and set them back to their original state.

tests/test_utils.py Outdated Show resolved Hide resolved
@bigtedde
Copy link
Collaborator Author

bigtedde commented Aug 7, 2023

@real-yfprojects Please let me know if anything else is stopping this from being merged 👍

@m3nu
Copy link
Contributor

m3nu commented Aug 9, 2023

Some merge conflicts.

@bigtedde
Copy link
Collaborator Author

bigtedde commented Aug 9, 2023

Some merge conflicts.

Fixed merge conflicts, reduced superfluous code in misc_tab, and streamlined testing.

@m3nu
Copy link
Contributor

m3nu commented Aug 10, 2023

Works well and solves the problem we had after adding this feature. 👏

I do think we can do better on the tooltip and label. Here my suggestion:

Label: "Use the same unit of measurement for archive sizes"
Tooltip: "When enabled, all archive sizes will use the same unit of measurement, like MB or KB. Can make reading sizes easier to read."

Also pinging @hariseldon78 for his 5 cents.

'tooltip': trans_late(
'settings',
'When enabled, all archive sizes will use the same unit of measurement, '
'such as KB or MB. This can make archive sizes easier to compare.',
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used the word "compare" instead of "read", since that might to be closer to the original intention of the setting.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that!

@m3nu m3nu merged commit b58ffb6 into borgbase:master Aug 11, 2023
39 checks passed
@m3nu
Copy link
Contributor

m3nu commented Aug 11, 2023

Thanks for the work, @bigtedde ! Very nice addition.

@hariseldon78
Copy link
Contributor

hariseldon78 commented Aug 11, 2023

Also pinging @hariseldon78 for his 5 cents.

I think the text is good and i'm sorry i had to abandon this feature to its destiny, work is increasing costantly and filling all the spare time... Thanks for taking care of improving it!

@bigtedde bigtedde deleted the archive_units_setting branch August 11, 2023 17:03
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.

FR: Setting for the new number format used in the size column of the archive tab
4 participants