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

Allow changing width of music mode window (fixes #3484) #4381

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

svobs
Copy link
Contributor

@svobs svobs commented Apr 22, 2023


Description:

This patch:

  • Allows resizing the width of the music mode window up to 1000px (which can be changed via a hidden pref key).
  • Fixes bugs which prevented the restore of location & size of music mode window across launches in all its configurations (with album art hidden or shown, playlist hidden or shown).
  • Retains height of the music mode playlist across toggles and across app launches
  • Changes window resize behavior so that resizing no longer hides/shows the playlist; it has to be explicitly toggled. Also, playlist now has a minimum height so that the window can be resized reliably.
  • Fixes numerous other small layout issues/glitches relating to music mode.

@low-batt
Copy link
Contributor

It acts weird for me. Starting from this:
issue-3484-normal

As I drag it wider the playlist gets shorter and then disappears when release the mouse button:
issue-3484-wide

Shouldn't this maintain the 50/50 split between the cover and the playlist?

@svobs
Copy link
Contributor Author

svobs commented Apr 23, 2023

I'm not sure I would expect it to maintain a 50/50 split exactly. If it were left to me, personally I might want to grow the playlist along with the title art until the window has filled up all vertical space on the screen, and then continue to allow the album art to grow at the expense of the playlist. But that kind of behavior involves getting the window and screen involved in the layout strategy, which is like the next level down in the Auto Layout dungeon for me...

I was about to suggest that I put in a minimum playlist size so that the album art doesn't eat up the playlist completely, but then I noticed that the playlist is set to auto-hide itself if its vertical height it reduced to less than ~5 rows. So now I think this might actually be consistent with the original intent...as though the playlist should be thought of as an optional add-on, and treated secondary to the player. Or maybe I'm over-analyzing.

Personally I think the current result is fine, but I think the decision really comes down to design intent. Would be great to get more input from the project owners on this.

@uiryuu
Copy link
Member

uiryuu commented May 3, 2023

The result is good! But we might want to set a maximum width, since if the width is too large, the UI beings to falling apart; besides it looks weird if the width is too large.

@svobs svobs force-pushed the pr-allow-music-mode-resize branch from 9a3ecc6 to fd19dd8 Compare June 8, 2023 09:09
@svobs
Copy link
Contributor Author

svobs commented Jun 8, 2023

The result is good! But we might want to set a maximum width, since if the width is too large, the UI beings to falling apart; besides it looks weird if the width is too large.

Well, since you didn't give a number, I gave it a nice max width of 700. That's just the number I settled on so that all of the music tracks I was tested with were able to show both the artist & the song title without truncating them.

I also kinda went overboard and make more fixes and tweaks:

  • Reduced the playlist auto-hide threshold by about 30%, so that it will hide if it is too short to show less than 4 items. It didn't seem reasonable to need so many lines, especially at lower resolutions (though maybe there is a good argument for it?)
  • Added code to prevent the player controls from being pushed off the bottom of the screen when the user expands the width of the window.
  • Reworked the logic which determined whether the playlist was shown or hidden, as well as the toggle action, because it had some problems which were causing buggy behavior. Also changed the names for a few of the window-related functions to be much clearer.
  • Size & position of the music mode window is now restored from the last launch.
  • Also fixed a constraint violation when opening music mode at the lowest resolution (1168x755 in my testing).

The one thing I'd still like to do is add logic so that when expanding the window width, the window's height will also expand so that the playlist won't get eaten up if there's space for it.

@svobs svobs marked this pull request as draft June 8, 2023 09:46
@svobs
Copy link
Contributor Author

svobs commented Jun 8, 2023

Reduced the playlist auto-hide threshold by about 30%, so that it will hide if it is too short to show less than 4 items. It didn't seem reasonable to need so many lines, especially at lower resolutions (though maybe there is a good argument for it?)

I already see I messed this up. I'll see if I can improve it.

@svobs svobs force-pushed the pr-allow-music-mode-resize branch 2 times, most recently from 5763bf7 to b64a47f Compare June 8, 2023 20:43
@svobs
Copy link
Contributor Author

svobs commented Jun 8, 2023

Pushed another update which fixes the mentioned bugginess with the auto-hide threshold and also improves timing of the playlist toggle when the button is clicked multiple times quickly (although it's not quite perfect...it sometimes will collapse unexpectedly. It's pretty hard to get this right without using a lock or a queue...). Also now the playlist height is saved, so when toggling it to hide and then show again, it goes back to its previous height instead of the default height.

@svobs svobs marked this pull request as ready for review June 9, 2023 06:05
@svobs
Copy link
Contributor Author

svobs commented Jun 9, 2023

Lots of work later, I think I've squashed all the immediate bugs and I'm pretty happy with the result. Feedback would be good.

I made a new behavioral change in the most recent commit which may or may not be appreciated. I changed the resize behavior so that when a playlist is showing, the only way to truly get rid of it is to toggle the "Playlist and chapters" button (seems like the Show/Hide Playlist Panel menu item should do something, no?), because I made it impossible to shrink the playlist below its minimum height. Similarly, when the playlist is hidden, resizing the window won't bring back the playlist; it will need to be toggled on. I think this is more in line with other media players and is a little more intuitive for the user, although maybe more annoying for power users because now they can't resize the window + toggle the playlist in the same click.

@svobs svobs force-pushed the pr-allow-music-mode-resize branch 3 times, most recently from 16e0bc4 to 89079bc Compare June 9, 2023 07:18
@svobs svobs marked this pull request as draft June 13, 2023 21:21
@svobs
Copy link
Contributor Author

svobs commented Jun 13, 2023

I noticed now that I can't assume the album art / video is square; it may be any aspect ratio. This complicates things a bit more.

Also, the size and location of the window is not restored correctly across launches if the album art was hidden. This is an existing issue in develop, but the way the window is being built is hard-coded to include the album art and this breaks Cocoa's autosave/autorestore functionality. Fixing this would likely require redoing the whole layout of the window. Will continue to evaluate...

@svobs
Copy link
Contributor Author

svobs commented Jul 14, 2023

Also, the size and location of the window is not restored correctly across launches if the album art was hidden.

Found a surprisingly easy fix for this (latest commit). Just need to get around to figuring out the album art aspect ratio...

@svobs svobs force-pushed the pr-allow-music-mode-resize branch 3 times, most recently from 4bc5ce6 to 65c231e Compare August 3, 2023 23:31
@svobs svobs marked this pull request as ready for review August 3, 2023 23:33
@svobs svobs changed the title Allow user to resize the width of the player in Music Mode (fixes #3484) Allow changing width of music mode window (fixes #3484) Aug 3, 2023
@svobs
Copy link
Contributor Author

svobs commented Aug 3, 2023

This turned into an enormous task. Handling window position & sizing in AppKit feels like trying to assemble IKEA furniture without instructions, using only rocks and tree branches as tools. Or trying to build a modern website using only the C standard library. Maybe there are some secret APIs which I'm not aware of, but it's a hard world to work in. Some parts touched the boundaries of my math skills.

There are a couple minor imperfections still, but I don't think they should be noticable. Will see what others think. And need to know if I missed something or regressed due to merge. There were so many little corner cases.

@svobs svobs force-pushed the pr-allow-music-mode-resize branch from 65c231e to 188f39d Compare August 4, 2023 01:02
 - Enforce separate resize logic when showing vs. not showing the playlist. When showing the playlist, playlist cannot be reduced to less than its minimum size.  When not showing the playlist, resizing alone will not show the playlist.
- Save & restore the size & position of the music mode window between launches. Save state of playlist visibility after resize. Save playlist height in prefs. Make max width of music mode window into pref. Fix bugs preventing restore of window size & position when videoView not shown.
- Fix "toggle playlist" button logic so it works more reliably.
- Add logic to prevent play controls from being pushed off screen during resize.
- Fix unwanted highlight of tabs
- Fix incorrect aspect ratio of video when changing tracks
- Refactor ScrollingTextField to support variable window width.
@lhc70000
Copy link
Member

I feel like this commit contains too much things. We want to allow changing music mode window width in 1.4.0; however, this one:

Changes window resize behavior so that resizing no longer hides/shows the playlist; it has to be explicitly toggled. Also, playlist now has a minimum height so that the window can be resized reliably.

when implementing this, we were trying to mimic iTunes / Apple Music's mini player. We need to discuss more about whether we want or don't want such behavior.

It would be great if you can prepare a PR that only contains other fixes. My apologies for the late reply and the conflicts... I'm considering getting rid of xibs in this project.

@iina iina deleted a comment from uiryuu Jun 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants