Skip to content

Ensure the progress dialog show how progress is measured #18341

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Arthur-Milchior
Copy link
Member

While doing a full sync, I found absurd to see the exact number of bytes of the collection. With 11 digits (100 mb), it was actually hard to even see how much progress was done, as it would require being able to count whether progress currently has 9, 10 or 11 digits.

Using kb, mb makes it more readable. I don't expect that GB will currently be useful, but it's still worth adding to be future proof. I don't expect this extra translation will be painful.

At the same time, I added the note that something is about a number of notes.

I also added an option for scalar progress when no relevant type can suffixed to the number. This is the case in particular for database check, where we have a number of very distinct steps. In this case, I still let the translators translate "current/max" because I'm not certain this is universal way of showing progress

Screenshot_20250518_230020

Copy link
Contributor

Important

Maintainers: This PR contains Strings changes

  1. Sync Translations before merging this PR and wait for the action to complete
  2. Review and merge the auto-generated PR in order to sync all user-submitted translations
  3. Sync Translations again and merge the PR so the huge automated string changes caused by merging this PR are by themselves and easy to review

Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

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

This feels overly complex, I suspect it's much easier to understand with an optional formatProgressAmount method

// Using Long in case a collection is more than 4gb
data class Amount(
/**
* Represents the current progress. It's expected to evolve.
Copy link
Member

Choose a reason for hiding this comment

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

I don't know what this means

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed the second sentence.
When I created it, I expected that the amount value would be set only once, and only the current property would be changed.
I realized later that actually, given the way the code is written, this would be complex to make this change, and totally useless.

Comment on lines 104 to 107
<string name="byte_progress" comment="Indicates to the user how many bytes have been processed out of a known amount of bytes Consider keeping the unbreakable space after the number.">%1$d/%2$d bytes</string>
<string name="kilobyte_progress" comment="Indicates to the user how many bytes have been processed out of a known amount of kilobytes Consider keeping the unbreakable space after the number.">%1$d/%2$d kB</string>
<string name="megabyte_progress" comment="Indicates to the user how many bytes have been processed out of a known amount of megabytes Consider keeping the unbreakable space after the number.">%1$d/%2$d MB</string>
<string name="gigabyte_progress" comment="Indicates to the user how many bytes have been processed out of a known amount of gigabytes Consider keeping the unbreakable space after the number.">%1$d/%2$d GB</string>
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

@Arthur-Milchior Arthur-Milchior May 18, 2025

Choose a reason for hiding this comment

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

Thank you, I didn't know about it.

However, I admit it's not clear to me it's actually better as it leads to sentences such as 512 kB/ 3MB.
I like the fact that, in my implementation, both numbers would be in the same unit, and the unit would not be repeated.

Arthur-Milchior added a commit to Arthur-Milchior/Anki-Android that referenced this pull request May 18, 2025
As a matter of styleguide, I recall it was recommender that a number
using digits should be on the same line as the thing being counted. I
found in Ankidroid a line ending with numbers

While creating ankidroid#18341 I found that  we may end up with lines ending
with numbers.

I don't know whether this rule is universal, so I only add a comment
requesting translators to consider this. Still, I believe it's useful
to indicate that this is an option as translators may not know it.
@Arthur-Milchior
Copy link
Member Author

I admit I don't understand what formatProgressAmount would be. Would it be a method that replaces progressSuffix?
I admit I like the fact that the type can be re-used over other progress. Even if we don't actually have many progress using an actual amount.

Arthur-Milchior added a commit to Arthur-Milchior/Anki-Android that referenced this pull request May 20, 2025
As a matter of styleguide, I recall it was recommender that a number
using digits should be on the same line as the thing being counted. I
found in Ankidroid a line ending with numbers

While creating ankidroid#18341 I found that  we may end up with lines ending
with numbers.

I don't know whether this rule is universal, so I only add a comment
requesting translators to consider this. Still, I believe it's useful
to indicate that this is an option as translators may not know it.
github-merge-queue bot pushed a commit that referenced this pull request May 24, 2025
As a matter of styleguide, I recall it was recommender that a number
using digits should be on the same line as the thing being counted. I
found in Ankidroid a line ending with numbers

While creating #18341 I found that  we may end up with lines ending
with numbers.

I don't know whether this rule is universal, so I only add a comment
requesting translators to consider this. Still, I believe it's useful
to indicate that this is an option as translators may not know it.
@mikehardy mikehardy requested a review from david-allison June 2, 2025 16:43
@david-allison
Copy link
Member

It'd be nice to find a second reviewer, I feel this is too complicated, and want to be sure I'm not asking for too much.

Having two units doesn't bother me, and feels natural:

Screenshot 2025-06-02 at 18 21 53

@Arthur-Milchior
Copy link
Member Author

I did the change you requested. I’m not blocking on units. Anyway, I still believe it’s a net improvement over the current dialog

@david-allison
Copy link
Member

david-allison commented Jun 3, 2025

I still feel this is too complex for formatting a string and would like a second opinion

In particular, this feels like one rare place in the code where we want to avoid allocations

nit: In addition: 1/1 notes

While doing a full sync, I found absurd to see the exact number of
bytes of the collection. With 11 digits (100 mb), it was actually hard
to even see how much progress was done, as it would require being able
to count whether progress currently has 9, 10 or 11 digits.

Using kb, mb makes it more readable. I don't expect that GB will
currently be useful, but it's still worth adding to be future proof. I
don't expect this extra translation will be painful.

At the same time, I added the note that something is about a number of
notes.

I also added an option for scalar progress when no relevant type can
suffixed to the number. This is the case in particular for database
check, where we have a number of very distinct steps. In this case, I
still let the translators translate "current/max" because I'm not
certain this is universal way of showing progress
@Arthur-Milchior
Copy link
Member Author

Sorry, but what do you mean by allocation?
Usually when coding, I know "allocation" for resource/memory allocation, but that does not seems relevant here

@Arthur-Milchior
Copy link
Member Author

1/1 note is not relevant anymore. There was change in the code and it seems we are not counting note anymore. The only use was removed in 9eeef01

@david-allison
Copy link
Member

david-allison commented Jun 16, 2025

Memory allocations - I believe progress is a tight loop


Still waiting for a second reviewer on this one. This still feels over-engineered to me, and I'm looking for someone to disagree

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.

2 participants