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 scrollbar overlapping icons in the toolbar container on some devices #411

Merged
merged 2 commits into from
Jan 13, 2024
Merged

Conversation

arcarum
Copy link
Contributor

@arcarum arcarum commented Jan 11, 2024

This PR aims to fix issue #354.

This PR forces some devices to use the normal scrollbar with the color/almost_background (which looks similar to the scrollbar in pixel devices).

@Helium314
Copy link
Owner

Does it also work if you set it to some semi-transparent color? A grey scroll bar may look out of place, depending on the keyboard background color.

@RHJihan does this fix the scrollbar placement issue for you?

@arcarum
Copy link
Contributor Author

arcarum commented Jan 12, 2024

Any color should work but do you have anything in mind? I chose grey because it was what looked closest to the default scrollbar used on pixel devices. I tried some of the ones that looked semi-transparent but they are usually not visible on either light or black themes.

@RHJihan
Copy link
Contributor

RHJihan commented Jan 12, 2024

@RHJihan does this fix the scrollbar placement issue for you?

Yes, it solves the issue.

@BlackyHawky
Copy link
Contributor

Any color should work but do you have anything in mind?

Maybe doubleAdjustedBackground would be nice?

@arcarum
Copy link
Contributor Author

arcarum commented Jan 12, 2024

I think only colors in colors.xml can be used. As far as I can tell doubleAdjustedBackground can't be used like @color/almost_background

@arcarum
Copy link
Contributor Author

arcarum commented Jan 12, 2024

It works if I set it programmatically but it is almost invisible with the black theme. I don't mind using it though.

@BlackyHawky
Copy link
Contributor

BlackyHawky commented Jan 12, 2024

It works if I set it programmatically but it is almost invisible with the black theme. I don't mind using it though.

Good news.

Perhaps you could try something like this in the Colors.kt file:

SCROLLBAR_THUMB -> if (isDarkColor(background)) Color.WHITE else doubleAdjustedBackground

Edit: Not in the DynamicColors class; I think this class will be able to use doubleAdjustedBackground every time.

@arcarum
Copy link
Contributor Author

arcarum commented Jan 12, 2024

I am not sure about setting it programmatically because setHorizontalScrollbarThumbDrawable only works on API 29+. Also I think using the same color as KEY_HINT_TEXT would look better and keep things simple.

@BlackyHawky
Copy link
Contributor

That's what I thought.

Have you tried this?

@arcarum
Copy link
Contributor Author

arcarum commented Jan 12, 2024

I did not try it but it seems unnecessarily complex for what can be a simple fix. If grey is not suitable then a different color can be added to colors.xml and used. I personally think that the scrollbar without this PR looks grey on non-samsung devices so a grey color is fine (and some other apps usually have a grey scrollbar).

@BlackyHawky
Copy link
Contributor

I couldn't agree more.
It was just an idea.
Personally I don't activate all the icons so I have the space so I never see the scroll bar.

@Helium314
Copy link
Owner

Any color should work but do you have anything in mind?

Definitely a transparent color, as mentioned. Maybe grey or dark grey is ok.

I chose grey because it was what looked closest to the default scrollbar used on pixel devices.

The pixel device scrollbar is always grey? imo that looks really ugly when using a colored keyboard background. On LineageOS 16 it appears to be some transparent grey, so the scroll bar is just a little darker than the background.

@arcarum
Copy link
Contributor Author

arcarum commented Jan 12, 2024

Is this good?

@Helium314
Copy link
Owner

I guess that's ok...
Thanks for fixing!

@Helium314 Helium314 merged commit 0eab3c3 into Helium314:new Jan 13, 2024
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.

4 participants