-
-
Notifications
You must be signed in to change notification settings - Fork 388
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
3335: Use different bounding aspect ratios for landscape #3396
base: develop
Are you sure you want to change the base?
Conversation
app/src/main/java/com/keylesspalace/tusky/util/AttachmentHelper.kt
Outdated
Show resolved
Hide resolved
val deviceIsHigher = displayMetrics.heightPixels > displayMetrics.widthPixels | ||
|
||
val minAspect = if (deviceIsHigher) 0.5 else 1.2 | ||
val maxAspect = if (deviceIsHigher) 2.0 else 3.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why specifically these constants for landscape?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to reverse "the idea of 0.5 to 2 for a portrait phone". However the literal 1 to 4 is probably a bit too much.
So these are "best and simple guesses for a wider range of landscape".
Generally one could discuss if this should be relative to the actual aspect ratio value of the device. (After all simply "height > width ?" is somewhat generic)
However I would venture that this is a simple solution.
(1.2 and not 1 because at least phones are nowadays much much higher than wide.)
Do you have some before/after screenshots of this? |
72ac2ef
to
c24fe04
Compare
Small counterpoint to @Lakoja 's remark - I actually do use Tusky on my phone in landscape, or, well, used to before the recent aspect ratio update. There's times it's more convenient to have my phone (Pixel 4 XL) in landscape orientation (e.g. propped up on a charging mount, so I'd need to pick it up and hold it instead), and I'd prefer to have smaller thumbnails for landscape even if it's not as usable as it would be on a tablet. That said, feel free to do what you wish with this change. |
But but but... Do you have a screenshot for me? |
Why is this still draft? Its definitely an improvement, would merge. |
Not quite sure if it is an improvement for landscape tablet resolutions (which have quite ok-ish height). But I un-drafted it now. Maybe somebody (more than one) would like to test it and if necessary we can fine-tune then. |
c24fe04
to
32dd3e0
Compare
To confirm, what kind of testing in landscape mode would be helpful to get this merged? I have a Pixel 4 XL and some older, rather slow devices (Nexus 6 on Android 8.1, Galaxy Nexus on Android 6.0). To get an APK, I assume I'd just need to follow the usual Tusky development build steps with this PR as the base. |
Hi. Thank's for your offer. However, I think this might be (discussed internally and) merged, so it can be tested more easily from the nightly version. |
32dd3e0
to
0f9736f
Compare
Do we need more testing on this? |
Probably not (merge it). I tested this again and see three classes of devices:
So the largest portion of devices including all tablet orientation do not have a problem. Only the landscape phones would benefit here. So, if we want to merge this we would first need to specifically only target landscape phones. |
Improves / fixes #3335