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: Enable multi-line support for the deck picker footer #17349

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

Conversation

Aditya13s
Copy link
Contributor

@Aditya13s Aditya13s commented Nov 2, 2024

Purpose / Description

The purpose of this change is to allow the footer of the deck picker to display multiple lines of text.

Fixes

Approach

The setSingleLine method has been removed from the DeckPicker.kt file to allow the footer text to display across multiple lines. Additionally, a .replace function was implemented to ensure that newline characters in the text from sched.studiedToday() are replaced with spaces, preventing any text from being hidden.

How Has This Been Tested?

This change has been tested using different font sizes and languages.

Screenshot

multiline_footer

Checklist

  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • You have commented your code, particularly in hard-to-understand areas
  • You have performed a self-review of your own code
  • UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
  • UI Changes: You have tested your change using the Google Accessibility Scanner

Copy link
Member

@Arthur-Milchior Arthur-Milchior left a comment

Choose a reason for hiding this comment

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

Thank you very much. I appreciate the improvement.

I'll admit that, having the text below the + symbol is not ideal. It's still readable, and I don't expect that this bug is worth doing more effort to find a better design to solve it!

AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt Outdated Show resolved Hide resolved
Copy link
Member

@BrayanDSO BrayanDSO left a comment

Choose a reason for hiding this comment

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

Kind of similar to what was proposed in #16965, so the comments about the screen edges made by @lukstbit still stands.

Also, the screenshot in the PR description evidently shows that the text can be hidden by the Add button

@BrayanDSO BrayanDSO added the Needs Author Reply Waiting for a reply from the original author label Nov 2, 2024
@Aditya13s
Copy link
Contributor Author

Kind of similar to what was proposed in #16965, so the comments about the screen edges made by @lukstbit still stands.

Also, the screenshot in the PR description evidently shows that the text can be hidden by the Add button

As @Arthur-Milchior mentioned, this bug isn’t worth further effort and can be addressed in #17334, which completes the migration to edge-to-edge.

@Arthur-Milchior
Copy link
Member

@Aditya13s I don't have more authority than @BrayanDSO. I have approved this, but we usually wants to reviewer, and so if the other reivewer does not appreciate, this is up to discussion.
I'm pretty sure Brayan was able to read what I wrote, and simply disagree. He has good argument I didn't even know about (edge-to-edge).

@BrayanDSO Can I ask what would be your preferred solution regarding the button?
Moving it, by ensuring it's above the text? Having the text width be small enough that it does not overlap with the button? (I'd fear we end up needing 3 lines if we don't allow the text to cover the full width of the screen)
I guess we can use Rect.intersects on the rectangle of the text view and the FAB to check whether they intersect, and reduce the width only if it's the case. Not very elegant but it would work.

can be addressed in #17334, which completes the migration to edge-to-edge.

Yeah, but in this case, there is not even a point of landing your change if we suspect we may need to change the result soon after. We should try to fix #14641 simultaneously, given that it consider the same part of the UI.

To be clear, I still believe that your change is an improvement compared to the current state. After all, the total quantity of missing text is quite smaller. But I prefer to have a single merged change that deals with all the issues related to this text view simultaneously

@Aditya13s Aditya13s force-pushed the multiple_lines_footer branch from 53340f2 to 7968085 Compare November 3, 2024 16:45
@BrayanDSO
Copy link
Member

Moving it, by ensuring it's above the text?

Yes. I believe that it can be easily done by editing only the layout files. There shouldn't be a need to do it programmatically

Copy link
Member

@lukstbit lukstbit left a comment

Choose a reason for hiding this comment

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

this bug isn’t worth further effort and can be addressed in #17334, which completes the migration to edge-to-edge.

Edge to edge becoming the default affects the interaction between the info text and the screen borders. The bug in the picture isn't related to edge to edge, it's related to the changes you've made in this PR and the interaction between the info text and the fab. So the current "fix" only fixes the issue partially and would result in a new bug report in the near future.

@Aditya13s
Copy link
Contributor Author

Screenshot From 2024-11-12 02-05-42

Setting the CoordinatorLayout elevation of the Floating Action Button (FAB) to -1dp gives the right look, but it makes the FAB unclickable because it ends up behind other views.

@lukstbit
Copy link
Member

There should be enough space made available for the info text between the screen margin and fab button. It shouldn't be visible below or above the fab button.

@Aditya13s Aditya13s force-pushed the multiple_lines_footer branch from 7968085 to 0f1454d Compare November 12, 2024 16:51
@Aditya13s
Copy link
Contributor Author

footer
This is the final output

@Aditya13s Aditya13s requested a review from lukstbit November 12, 2024 17:05
@Arthur-Milchior
Copy link
Member

Clearly this result looks nicer. I admit I regret that it moves the FAB even when the text is small. I'd have preferred if we had a way to ensure it remains at the same place if possible.

I admit I really don't know XML layout enough to know how to do that.
I guess we should have a RelativeLayout that contains the today_stats_text_view at the bottom, and a layout that contains both the deck list and the fab at the top.
But that needs a serious rewrite of deck_picker.xml

@Aditya13s Aditya13s force-pushed the multiple_lines_footer branch from 0f1454d to 9aed7c9 Compare November 13, 2024 07:38
@Aditya13s
Copy link
Contributor Author

Clearly this result looks nicer. I admit I regret that it moves the FAB even when the text is small. I'd have preferred if we had a way to ensure it remains at the same place if possible.

Fixed it by dynamically adjusting the layout's bottom margin. If the line count is greater than 1, the margin changes; if it's 1, no change occurs.

@Aditya13s Aditya13s requested a review from BrayanDSO November 14, 2024 12:03
@Aditya13s Aditya13s force-pushed the multiple_lines_footer branch from 9aed7c9 to 1452476 Compare November 14, 2024 18:27
@Aditya13s Aditya13s requested a review from BrayanDSO November 14, 2024 18:27
@Aditya13s
Copy link
Contributor Author

@BrayanDSO Please review the changes

@BrayanDSO BrayanDSO added Needs Review and removed Needs Author Reply Waiting for a reply from the original author labels Nov 19, 2024
@Aditya13s Aditya13s force-pushed the multiple_lines_footer branch from 1452476 to c209a62 Compare December 14, 2024 18:13
@Aditya13s Aditya13s force-pushed the multiple_lines_footer branch from c209a62 to c895a76 Compare December 14, 2024 18:38
@Aditya13s
Copy link
Contributor Author

@lukstbit Merge Conflicts Resolved. Please review this.

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.

[BUG]: String of review count at bottom of deck picker screen overflows
4 participants