search

sghpjuikit / player

Audio player and management application.
22 stars 2 forks source link

Playback widget doesn't handle small sizes #112

Closed xeruf closed 5 years ago

xeruf commented 5 years ago

image

I guess this is a general issue of components, but it matters especially for Playback. I was thinking that one could unify Playback and Playback Mini, i.e. if the height is under a certain threshold, the Playback widget automatically enters mini mode.

sghpjuikit commented 5 years ago

PlaybackMini exists because it supports HorizontalDock, which means it can be put into tiny horizontal spaces (currently used for the docked mode).

I can reproduce the behavior. The layout for height would require some serious thinking and effort. But one way or another it would have to be reactive. This is a no go for many reasons.

The idea of merging the two widgets is sound and I like it. It is not easy to implement (much easier than reactive layout above). The implementation would be like this: two set of UIs would be created and depending on the height, one would display the desired one.

The implementation difficulty lies in

xeruf commented 5 years ago

But one way or another it would have to be reactive. This is a no go for many reasons.

Are there more reasons than the ones you already mentioned? This doesn't sound too bad to me.

When the ui switches, one must stop the binding for the old ui and start binding for the new. Basically, this requires refactoring both widgets to standalone classes with a dispose() method.

I was thinking about actually unifying and then reusing most nodes, this would also allow more than two layouts to be set. But I guess this is the easier way.

sghpjuikit commented 5 years ago

Well you do have a bright idea about reusing the nodes for the layout. Seems natural I know. But from my experience of doing just that - it is absolute nightmare. Because the layout is "reactive" you need to do it yourself (which is already very difficult). Once you add that reactivity, that under certain conditions the layout works differently, you open yourself to state management and depending on what exactly the layout is it may get complex fast.

I'm not saying it is impossible or even very difficult, but it can be but the greatest cost is in maintenance/readability. The more reactive layout you do, the less declarative the code is, because its no longer 'layout', but behavior. This is the reason I think having two sets of graphics and switching them up has potential to eliminate lots of inherent problems with the 'raw' layout approach. It does give you freedom, but you loose functionality like 'expansion', 'anchoring' and stuff. In order to regain that one ends up creating all sorts of partial layouts and it becomes unmanageable mess of Pane variables.

I did this 'raw' thing for FileInfo widget (which I urge you to take a look at) - it features a smart layout that resizes cover just the right way and then fills in the remaining space according to some rules about columns and minimum/maximum sizes. It works and I'm happy and it was nightmare (and its still bugged) and I never want to do it again, heh.

In the end I can only guess what the best approach here is. I do not have a clear idea whether current PlayerControls layout is any good either...

sghpjuikit commented 5 years ago

I started working on this. After removing fxml and some refactoring, I ran into the wall about how to approach the ui switching.

Reusing nodes has the disadvantage of having to somehow repopulate the layouts, which would involve a lot of getChildren().add(position, node). Having separate layouts does seem worse though. I think I will attempt a custom Pane with overriden layoutChildren(). This will be cleanest, but lose the ui being declarative.

sghpjuikit commented 5 years ago

Implemented in 6a6e25cc3a0f437a13ec8b27104b9fd4fac3986a.

Final implementation is best of both worlds. I reused all nodes, but still provide two separate layouts. This is so the big layout is defined declaratively, while the small is using overriden layoutChildren(), hence the small layout is only a single pane object. Only minor issue is layout repopulating, which could be improved using own class to store all cross-layout nodes.

Bottom line: No additional nodes or panes are used compared to old version and layout is still declarative where it needs to be, which means readable and flexible code.