3335: Use different bounding aspect ratios for landscape #3396
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.
Why specifically these constants for landscape?
There was a problem hiding this comment.
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? |
Lakoja
changed the title
Lakoja
force-pushed
the
3335-tweak-landscape-cropping
branch
from
72ac2ef to
c24fe04
Compare
|
So far I was only looking at phone resolution (in landscape). But there I think Tusky is generally unusable, regardless of used image crop aspect ratios (see screenshots in issue). It looks ok on a tablet because that does not have a full width display. So I guess this could be closed.
|
|
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? |
Ack, pardon, I missed the "wanting a screenshot" part. Here's an example of being able to read a full text post in landscape comfortably with the current stable Tusky release: I don't have the old version of Tusky archived in order to get a screenshot of how images looked. (In my case, I'm using Android's Gesture Navigation with the navigation hint hidden via LineageOS settings. There's a Magisk mod to do that on stock Android builds, too. Even without that, it only would've partially cropped out the interaction buttons, none of the actual post.) |
|
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. |
Lakoja
changed the title
Lakoja
force-pushed
the
3335-tweak-landscape-cropping
branch
from
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. |
Lakoja
force-pushed
the
3335-tweak-landscape-cropping
branch
from
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