Fix landscape scaling by using smallest dimension for density scaling#3933
Fix landscape scaling by using smallest dimension for density scaling#3933kairosci wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThis PR adds a user-configurable landscape scaling feature that adapts UI density based on the smallest screen dimension. A new preference key gates density scaling behavior; when enabled, UI density scales by 1.15f/1.1f/1.05f/1.0f based on screen size thresholds, otherwise stays at 1.0f. The preference is wired into settings with a toggle switch, and corresponding string resources are added. ChangesUI density scaling and settings
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@Nash20718 @E021ntox I created this to resolve the issue. Could you please confirm that it closes the issue? |
|
@kairosci I've tested this APK, and honestly, there's no difference from 13.5.0. I suggest not overcomplicating things here; the default 1.0 scaling already looks great in floating window, full-screen, and split-screen modes. Can you imagine a 13-inch screen showing only 6 rows of items in the settings menu? Also, please consider this as well. Thank you! #3386 (comment) |
kairosci
force-pushed
the
fix-landscape-scaling
branch
from
b9fec12 to
c86d0f8
Compare
kairosci
force-pushed
the
fix-landscape-scaling
branch
from
c86d0f8 to
758770c
Compare
kairosci
force-pushed
the
fix-landscape-scaling
branch
from
758770c to
3503b24
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
strip_scaling.py (1)
3-37:⚠️ Potential issue | 🟠 Major | ⚡ Quick winScript is incomplete and currently cannot perform any “strip scaling” transformation.
At Line [6],
out_linesis never populated, and from Line [34] onward the flow ends in a placeholderpasswith no output write, so running this file produces no usable result.Proposed fix
import re +from pathlib import Path -with open('app/src/main/kotlin/com/metrolist/music/MainActivity.kt', 'r') as f: +target = Path("app/src/main/kotlin/com/metrolist/music/MainActivity.kt") +with target.open("r", encoding="utf-8") as f: lines = f.readlines() out_lines = [] in_provider = False provider_indent = -1 skip_next = False +skip_brace_depth = 0 +provider_brace_depth = 0 -for i, line in enumerate(lines): +for line in lines: + if skip_brace_depth > 0: + skip_brace_depth += line.count("{") - line.count("}") + if skip_brace_depth <= 0: + skip_brace_depth = 0 + continue + if "val currentDensity = LocalDensity.current" in line: continue @@ if "val densityScale = remember(" in line: - skip_next = True + skip_brace_depth = max(1, line.count("{") - line.count("}")) continue - if skip_next: - if "}" in line and line.strip() == "}": - skip_next = False - continue if "val scaledDensity: Density = remember" in line: - skip_next = True + skip_brace_depth = max(1, line.count("{") - line.count("}")) continue if "CompositionLocalProvider(LocalDensity provides scaledDensity) {" in line: provider_indent = len(line) - len(line.lstrip()) + in_provider = True + provider_brace_depth = 1 continue - - # We need to drop the matching closing brace for CompositionLocalProvider - # But since it's hard to track in a simple loop without a stack, maybe just use regex on the whole content. - pass + if in_provider: + provider_brace_depth += line.count("{") - line.count("}") + if provider_brace_depth <= 0: + in_provider = False + continue + + out_lines.append(line) + +with target.open("w", encoding="utf-8") as f: + f.writelines(out_lines)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@strip_scaling.py` around lines 3 - 37, The script never builds or writes out_lines and exits with a placeholder pass, so implement full transformation: iterate through lines, append non-skipped lines to out_lines (use the existing flags out_lines, in_provider, provider_indent, skip_next), detect and drop the whole CompositionLocalProvider block (start when "CompositionLocalProvider(LocalDensity provides scaledDensity) {" is seen and skip until its matching closing brace using a brace counter or stack), skip the remembered density-related lines (the checks for "val densityScale = remember(", "val scaledDensity: Density = remember", etc.) as already started, and finally write the joined out_lines back to the same file; ensure variables and conditions (out_lines, in_provider, provider_indent, skip_next, CompositionLocalProvider) are used to locate and remove the intended sections.
🧹 Nitpick comments (1)
diff_to_apply.patch (1)
428-475: 💤 Low valueConsider extracting duplicated artist/otherInfo extraction logic.
The artist extraction (
when (item) { is SongItem -> ... }) andotherInfocomputation are nearly identical betweenYouTubeListItem(lines 365-389) andYouTubeGridItem(lines 441-466). Extracting these into helper functions would reduce duplication and simplify future maintenance.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@diff_to_apply.patch` around lines 428 - 475, You have duplicated logic for extracting artists and the "otherInfo" string in YouTubeGridItem that mirrors code in YouTubeListItem; refactor by creating helper functions (e.g., fun extractArtists(item: YouTubeItem): List<Artist>? and fun extractOtherInfo(item: YouTubeItem): String?) and replace the inline when blocks in both YouTubeGridItem and YouTubeListItem to call these helpers; ensure the helpers handle SongItem, AlbumItem, PlaylistItem, PodcastItem, EpisodeItem cases (matching the existing logic used for ClickableArtistText and the subtitle Text) so behavior remains identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@diff_to_apply.patch`:
- Around line 358-414: The badges call was moved inside the "if (item !is
ArtistItem)" branch so ArtistItem no longer renders badges; restore badge
rendering by invoking badges(this) unconditionally (move badges(this) outside or
before the ArtistItem check inside subtitleLambda) and also re-add the badges
parameter to the ListItem call (pass badges = badges) so ListItem receives the
badge content as before; key symbols: YouTubeListItem, subtitleLambda, badges,
ListItem, ArtistItem.
In `@test_animatable.kt`:
- Around line 7-16: Replace the ad-hoc main with an automated unit test: remove
or delete the top-level main and instead add a test function (e.g., fun
animatable_bounds_and_animateTo_throws_or_succeeds()) using your test framework
(JUnit or kotlin.test) that sets up Animatable(0.dp, Dp.VectorConverter), calls
updateBounds(0.dp, 100.dp) inside runBlocking, and then asserts the expected
outcome of animateTo(150.dp) (use assertFailsWith<...> if an exception is
expected or assertDoesNotThrow / success assertions otherwise); reference the
existing symbols Animatable, updateBounds, animateTo and runBlocking when
implementing the test so CI will fail if behavior regresses.
In `@test_content_length.kt`:
- Around line 3-5: test_content_length.kt currently contains only a placeholder
main function (fun main) that prints "Hello world"; either implement the
intended content-length test inside this file or remove the file from the PR to
avoid dead test artifacts. If implementing, replace or extend fun main to
include assertions that exercise the content-length logic under test (call the
target functions/classes and assert expected lengths), or convert it into a
proper unit test using your test framework; if not needed, delete
test_content_length.kt so the PR contains no placeholder tests.
---
Outside diff comments:
In `@strip_scaling.py`:
- Around line 3-37: The script never builds or writes out_lines and exits with a
placeholder pass, so implement full transformation: iterate through lines,
append non-skipped lines to out_lines (use the existing flags out_lines,
in_provider, provider_indent, skip_next), detect and drop the whole
CompositionLocalProvider block (start when
"CompositionLocalProvider(LocalDensity provides scaledDensity) {" is seen and
skip until its matching closing brace using a brace counter or stack), skip the
remembered density-related lines (the checks for "val densityScale = remember(",
"val scaledDensity: Density = remember", etc.) as already started, and finally
write the joined out_lines back to the same file; ensure variables and
conditions (out_lines, in_provider, provider_indent, skip_next,
CompositionLocalProvider) are used to locate and remove the intended sections.
---
Nitpick comments:
In `@diff_to_apply.patch`:
- Around line 428-475: You have duplicated logic for extracting artists and the
"otherInfo" string in YouTubeGridItem that mirrors code in YouTubeListItem;
refactor by creating helper functions (e.g., fun extractArtists(item:
YouTubeItem): List<Artist>? and fun extractOtherInfo(item: YouTubeItem):
String?) and replace the inline when blocks in both YouTubeGridItem and
YouTubeListItem to call these helpers; ensure the helpers handle SongItem,
AlbumItem, PlaylistItem, PodcastItem, EpisodeItem cases (matching the existing
logic used for ClickableArtistText and the subtitle Text) so behavior remains
identical.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 85256a13-6ed9-4e6d-91a3-eb808fb7ac74
⛔ Files ignored due to path filters (1)
video.mp4is excluded by!**/*.mp4
📒 Files selected for processing (11)
app/src/main/kotlin/com/metrolist/music/MainActivity.ktapp/src/main/kotlin/com/metrolist/music/constants/PreferenceKeys.ktapp/src/main/kotlin/com/metrolist/music/playback/MusicService.kt.origapp/src/main/kotlin/com/metrolist/music/ui/screens/settings/AppearanceSettings.ktapp/src/main/res/values/metrolist_strings.xmldiff_to_apply.patchpr_body.mdstrip_scaling.pytest_animatable.kttest_content_length.ktvol.txt
✅ Files skipped from review due to trivial changes (2)
- vol.txt
- pr_body.md
|
@E021ntox I added a toggle for enable it; I've also added validation so that it only works on tablets. If you'd like, give it a try and let me know what you think! |
It looks like there’s no compiled APK available for testing. |
#3386 (comment) I think this issue deserves more attention. |
|
yes, it is |
It's better now. |
|
So does that sound okay to you? |
Yes. |
|
Also, I would like to request improving the import feature once again. This is truly crucial for us; otherwise, we won't be able to switch to a new device. |
|
I still don't quite understand the issue with the import. Could you open a new issue where you explain it in more detail? |
|
The import function takes urlColumnIndex as a parameter, but this field is never actually used during the import process. Instead, it dynamically matches the video ID based on the title, which results in a high volume of incorrect matches. The proper approach for a clean playlist migration should be importing only the video IDs and then refetching the relevant metadata. I have already explained the rest of the logic in detail in my previous post.
|
|
The specific steps to reproduce this are very straightforward: just export your playlist to a CSV and import it on another device. You'll notice that the playlist becomes completely mangled, bearing no resemblance to your original one, and the tracks are completely out of order. |
|
On top of that, there are a few design issues. We're hoarding way too much data in the database, and honestly, a lot of it seems like stuff we don't even need to save. |
Like what? |
|
This button should be removed. I accidentally tapped it and lost my favorite songs without even realizing it. |
3 participants
Problem
The UI was excessively zoomed in when switching to landscape orientation on phones, making elements like lyrics difficult to view. Additionally, rotation transitions were sluggish.
Cause
The adaptive density scaling logic in
MainActivity.ktwas based solely oncontainerWidthDp. On phones in landscape, the width often exceeded 600dp, triggering tablet-level scaling (1.1x+). This increased density reduced logical vertical space and forced a full UI recomposition during rotation because the density value changed.Solution
Modified the scaling logic to use the smallest dimension (
minOf(width, height)) for determining the scale factor. This makes the scaling orientation-independent (following theswconvention) and ensures consistent density across rotations.Testing
Verified that phones (sw < 600dp) stay at 1.0x scale in both portrait and landscape. Large tablets still benefit from scaling as intended.
Related Issues
Summary by CodeRabbit