-
Notifications
You must be signed in to change notification settings - Fork 104
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
Improvement: Rework UI with focus on NowPlaying #1004
Improvement: Rework UI with focus on NowPlaying #1004
Conversation
662df26
to
8183e9e
Compare
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.
Fine in general, although it was quite difficult to review such a huge PR
there’s also the place with custom view as separator where you hide the view - seems that the separator can be removed completely instead
@@ -410,13 +417,15 @@ - (void)setButtonImageAndStartDemo:(UIImage*)buttonImage { | |||
} | |||
|
|||
- (void)animateToColors:(UIColor*)color { | |||
color = UIColor.clearColor; |
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.
Method parameter makes no sense now
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.
Yep. For now I do not want to change this further as I want to have the option to roll back easily, just in case. If we this will stay, I can remove further code which did the coloration of text in the UI.
@@ -710,13 +731,19 @@ - (void)getActivePlayers { | |||
repeatButton.hidden = NO; | |||
} | |||
if ([repeatStatus isEqualToString:@"all"]) { | |||
[repeatButton setBackgroundImage:[UIImage imageNamed:@"button_repeat_all"] forState:UIControlStateNormal]; | |||
UIImage *image = [UIImage imageNamed:@"button_repeat_all"]; | |||
image = [Utilities colorizeImage:image withColor:KODI_BLUE_COLOR]; |
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.
these 3 lines are a very common pattern now
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.
Yep, this I kept to not make the PR even bigger -- as you rightly said it is already quite huge and hard to review or to maintain. If ok, I will rework the repeat/shuffle coloration in a dedicated PR. There is even more code duplication which can be avoided as each block for repeat and for shuffle is coded twice in a very similar fashion.
XBMC Remote/NowPlaying.m
Outdated
[playlistToolbarView addSubview:repeatButton]; | ||
|
||
// Update layout of play control bar | ||
CGFloat buttonPadding = floor(playlistToolbarView.frame.size.width / 9); |
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.
What is 9?
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.
Thanks for pointing me to this one. There were few issues hidden behind this, I added a fixup commit for this which uses defines and now corrected the layout further.
button = [playlistToolbarView viewWithTag:TAG_ID_STOP]; | ||
button.center = CGPointMake(centerX - 3 * buttonPadding, button.center.y); | ||
button = [playlistToolbarView viewWithTag:TAG_SEEK_BACKWARD]; | ||
button.center = CGPointMake(centerX - 2 * buttonPadding, button.center.y); |
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.
Positioning these buttons should be done with a loop
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.
Let's please keep. Loops would need introduction of an array with view tags and relative positions in it. I doubt this would be easier to read.
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.
array (actually dictionary, as we can't create an array of pairs out of the box) would explicitly show which parameters change - this is way easier to read than the current wall of text :)
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.
Ok, will give it a try 😃
Shall I create GitHub issues to finish work on the colorization clean up and the repeat/shuffle button refactoring? |
f7ac7e4
to
c66e0e3
Compare
Now squashed the fixups and rebased to master. |
Introduces a new and centralized Play/Pause icon and new sleek shuffle/repeat icons. Moves the shuffle/repeat button to NowPlaying screen, aligns them horizontally with the play control buttons and accordingly adapts the overlay screen. The NowPlaying labels use white/gray color without font shadows.
For a solid animation experience when toggling iPhone between NowPlaying and playlist the view sizes must be adapted and the underLeft and underRight views must be hidden while animating.
Updates background of all views. For NowPlaying also removes the Kodi overlay icon. Background does now have correct dimensions and does not need to be resized during layout.
On iPad the shuffle/repeat buttons should have the same white color as the other play control buttons because they all reside in the same row. On iPhone the shuffle/repeat buttons are detached from the play controls and should be less prominent until activated.
Moves the play control view out of the toolbar and under the BottomView. This keeps the layout and spacing as it is defined for iPhone. Changes iPhone's playlist/NowPlaying button to iPad's fullscreen toggle button. On press this sends notification to iPad view controller and then animates to fullscreen view. The method to set the NowPlaying dimension is enhanced to handle both fullscreen and split screen.
The font scale factor is taken from the layout height calculation for BottomView which contains the labels. This way the initial vertical relative spacing between the labels is kept same.
Feature can be chosen in Kodi App settings, it is enabled by default. To improve readability of play controls and labels a dark blur effect with an additional gradient overlay is used. Changes between different covers are animated.
Avoids an overlap of toolbar and playlist action and fixes a visible gap between toolbar and play controls.
This button is placed in the iPad toolbar and therefor allows to close all stacked views at one click.
c66e0e3
to
07cfda8
Compare
@wutschel what about this? |
Wanted to ask about this anyhow. You remember by chance where you saw this? Edit: Nevermind, found it. This happens for at least two controllers. Will create an issue to not forget about it. |
Description
Closes #994
Closes #930
This PR brings all the UI rework around NowPlaying from the past months. The changes were tested via TF over few weeks and further changes were done based on user feedback. Overall this brings a fresher look for iPad and especially for iPhone/iPod with transparent top and bottom bars, an updated play control bar with highlighted central play/pause button, blurred cover, animation changes, alignment of icon colors, reduction overdone separator lines, removal of disturbing transparent Kodi logos and a new modern looking and common background for all menus.
Summary for release notes
Improvement: Rework UI with focus on NowPlaying
Feature: Add blurred cover feature for iPhone
Feature: Add fullscreen NowPlaying support for iPad
Improvement: Updated play controls with highlighted center play/pause
Improvement: Move iPad play controls to NowPlaying screen
Improvement: Shuffle/repeat now accessible from NowPlaying screen
Improvement: Introduce common gradient dark background for all menus