[CollapsingToolbarLayout] Allow multiple lines of text in expanded state #413
Conversation
|
As the Travis CI build is also failing on |
lib/java/com/google/android/material/internal/CollapsingTextHelper.java
Outdated
Show resolved
Hide resolved
lib/java/com/google/android/material/internal/CollapsingTextHelper.java
Outdated
Show resolved
Hide resolved
lib/java/com/google/android/material/internal/CollapsingTextHelper.java
Outdated
Show resolved
Hide resolved
|
@johan12345 When I run the demo in the catalog I see the text is black instead of white. Do you see that as well? |
|
It looks as though there might be a bug in RTL as well? If you enable pseudolocales for the catalog, it causes overlapping text: |
Yes, I see that as well, but the same happens with the original Collapsing Demo, also on |
@johan12345 Ah you can set a theme overlay on your AppBarLayout ( |
Okay, then I should better rebase onto master first, right?
Indeed, there's something wrong here. I will try to look into that... Probably, the calculations in CollapsingTextHelper.java#652 and CollapsingTextHelper.java#573 need to have a special case for RTL, but I'm not sure how that would need to be calculated to get the same result. I previously only tested the "force RTL layout" option in developer options, which looked fine. |
|
Hmm, I played around with the RTL issue for a couple of hours, but had no success so far. It's really difficult as while the text goes from right to left, the x-positions of things on the display still need to be calculated from the left... Some help would be appreciated! |
Yep I think the theme overlay was updated after this PR was created. |
johan12345
force-pushed
the
multiline-collapsingtoolbar
branch
from
87fbd71
to
7b08115
Compare
Okay, I rebased and applied the ThemeOverlay now 👍 |
Great. Oh and you should be able to remove the |
|
I got one part (position of the expanded text) fixed for RTL, but collapsed and cross-section text are still off... |
| if (isRtl) { | ||
| currentExpandedX = x + textLayout.getLineLeft(0) - expandedFirstLineDrawX * 2 + | ||
| currentBounds.width() - textLayout.getWidth() * scale; | ||
| currentCollapsedX = x; //+ currentBounds.width() - collapsedTextWidth * scale; |
There was a problem hiding this comment.
Thanks for working on this!
It looks like currentCollapsedX is always just x in the end like it was before? Also collapsedTextWidth isn't used because it's commented out here. But it does look like this does fix the x position of the expanded text.
There was a problem hiding this comment.
Yep, that was an unfinished try to also correct the position of the collapsed text, but it didn't work out this way.
I currently have a new approach where I try to adjust the calculateBaseOffsets function to directly calculate the correct separate x positions for the collapsed and expanded text (in LTR text they are the same, but not in RTL because they are aligned on the right side instead of the left) instead of adjusting them afterwards in draw(). Probably that approach is much better, but it's also not working correctly yet... should I push it to another branch so that you can take a look?
There was a problem hiding this comment.
I'm not sure when I'll be able to take a look to try to help identify the problem based on other priorities that we have. But I still think it would be useful to push what you have for now.
There was a problem hiding this comment.
Okay, I force-pushed the new approach here and kept the previous one as a backup on our multiline-collapsingtoolbar-old branch.
There was a problem hiding this comment.
Can you rebase the branches on master? We made a change recently which is preventing me from pulling them in. thanks!
There was a problem hiding this comment.
Okay, should now both be rebased. I accidentally committed the pseudoLocalesEnabled true in the build.gradle, that's probably what caused the conflict. Sorry!
johan12345
force-pushed
the
multiline-collapsingtoolbar
branch
3 times, most recently
from
b64febe
to
abc05e8
Compare
|
Any updates on this? |
|
@isaklilja As you can see, we are still trying to get it to work correctly with RTL text. I will be on vacation next week, so probably no more updates from me until after that. |
|
@johan12345 @cketcham Any new development on this. Waiting for the multiline support so that we can use in out app. Thanks. |
|
@shubh10 I have still not found a way to make it work correctly with RTL text. If any of you can help, that would be greatly appreciated. If you do not need RTL support, you can use our current version via JitPack for now, which is what we are also doing currently: // in your dependencies block, replace the material-components-android library with:
implementation 'com.github.opacapp:material-components-android:d290745a71'
// in your repositories block, add:
maven { url "https://jitpack.io" } |
|
Hi, I have rebased the branch onto the current master and moved our multiline code to a separate Of course, this results in some duplicated code, especially since But this should mean that the internal tests should work fine, as the default behavior will not change compared to the previous version. @ldjcmu, could you check this? |
|
I think that some duplication is fine for the rest of us who desperately need this feature built-in. You can create an improvement ticket to clean up the tech debt in the next release, and have a release for us? :) This PR has gone back and forth for a long time, perhaps its review can be prioritized? |
|
If we would fall back to start, that would mean that if the user specifies only the vertical gravity, this would be overridden. Therefore, I instead mutate the given value to only replace the byte for the horizontal gravity. |
|
@johan12345 I understand but you always either check with RELATIVE_HORIZONTAL_GRAVITY_MASK or RELATIVE_VERTICAL_GRAVITY_MASK and you have a default do the same as START so not sure why it's necessary |
|
I'm splitting this into more manageable pieces btw, no need to keep updating this pr. |
Hm, you're right. Maybe it was necessary earlier but not in the current state. |
|
I was just confuse for the fork library should I use this |
|
@ArcherEmiya05
|
One more thing, can I also add subtitle on the CollapsingToolbar? |
@ArcherEmiya05 That's not something our implementation currently supports. It might be possible to just add a custom TextView to the AppBarLayout for that, but I don't have an example handy. |
Thanks for the information :) |
|
Hi guys, is there chance that these changes might get merged anytime soon? Such a feature (multiline title) will be very handy. |
|
@bazinac There are issues with RTL text, which haven't been fixed yet. @johan12345 I've managed to add subtitle support as well. What do you think will be better? Finishing this PR up first, and then creating another one for subtitle support? Or shall we do it together? |
|
@johan12345 I finally got some time to work on this. Are you interested in continuing from 83e4e47 ? I think there might be a few bugs so I left the API restricted. The current implementation seems to work ok and it's not causing any screenshots changes on our side. Also RTL orientation actually works but an RTL language does not. Let me know if you can take a look and you have other thoughts. Don't think it's worth implementing jelly bean for now either. |
@adhirajsinghchauhan Well, in the end, that's something for Google to decide (@ymarian), but I would say this is already complicated enough that subtitles should better be done in a separate PR.
@ymarian
Okay, yeah, the demo should be quite straightforward to add.
Yes, I think that's what we had seen earlier as well - "force RTL layout" works, but RTL languages (+ pseudolocales) don't. RTL is more difficult for this, because in LTR the x positions (which are always calculated from the left) of the collapsed and expanded texts are the same, but not necessarily in RTL. That's why I tried to implement the calculation of the separate positions in opacapp@8f012b3, but I haven't got it to work unfortunately.
Yeah, that's probably fine. |
|
I would absolutely love to have subtitle solved as well. At the moment, we are using custom text view in the appBar and its not all that nice. I believe that subtitle would be oftentimes handy feature, due to the nature of the CollapsingToolbarLayout. Anyways, keep up the good work, I really appreciate your effort and I believe google does so as well :) |
|
@ymarian So, on a new branch, I have now added the maxLines attribute and the demo. |
|
@johan12345 Thanks a lot, that looks good to me, I can merge that in and close this PR. Any outstanding bugs can be addressed in different PRs or people can file bugs as they start to test. Sounds good? |
|
That sounds good, thank you! 🎉 |
which now contains a fully working implementation of the multiline collapsing toolbar, yay! 🎉 (see also material-components/material-components-android#413)
|
There was one functionality that was lost when integrating this library. I can have multiline text but the width is still limited because of the menu items as you can see here: In the multilinecollapsingtoolbar library the width was not limited which allowed a lot better design: |
|
@katajona Sorry, I'm no longer working at rami.io, the company who is developing the opacapp project and the multiline-collapsingtoolbar library, and making the calculations involved in in the multiline animation work correctly is quite complicated, so I unfortunately don't have time to look into this right now. I don't remember exactly anymore, but it could be that this functionality was removed either
|
closes #6 and was also discussed in https://issuetracker.google.com/issues/136120586, where you said you would welcome a PR.
We have implemented the possibility to use multiple lines of text as a title in the collapsing app bar. The additional lines are displayed in the expanded state and will be hidden in the collapsed state, like this:
This is based on the work of @raphaelm and me as well as a few contributors in the multiline-collapsingtoolbar library, which is based on the CollapsingToolbarLayout code of the Android Design Support library (predecessor of Material Components) and has already become quite popular.
However, as discussed in opacapp/multiline-collapsingtoolbar#62, it would be quite difficult to update our library with support for the new Material Components and AndroidX libraries, as many of the interfaces we are calling are not part of the public API and thus can only be called from the Material Components library itself. Therefore, it would be great if this feature could instead be included in the official Material Components library.
In addition to copying over the adjustments from our library into the CollapsingTextHelper codebase, I also added a demo in the catalog app to showcase this new feature.