Review Request 129695: [dragon] Simplified transition between different views

[Anthony Fieroni]

> On Дек. 23, 2016, 2:07 след обяд, Harald Sitter wrote:
> > Please run `rbt post -d` to try and get some debug information as to why reviewboard keeps rejecting your diffs. I'd like to take this to the sysadmins to get it fixed. Not being able to use the review software defeats the point of having review software. For the time being you can post your diffs on phabricator manually, so we can still discuss the diffs easily. https://phabricator.kde.org/differential/diff/create/
> > 
> > As for the diff itself:
> > Awesome change!
> > 
> > Functional problems:
> > - Play a video -> go to play media -> switch to an audio file -> preview widget still has a black background in the loadView (or white sometimes). I am not entirely sure why that is happening now and did not happen before, I am guessing this is actually a left over problem from the kf5 port. As an easy fix I think you can call setThumbnail on the loadView with a nullptr when entering stopped state.
> > - When you play a file -> go to play media -> now stop playback (s shortcut or menu) -> you'll now get toggled into the view where nothing happens. What it should do is stay on the loadview.
> > 
> > Style problems:
> > - `m_currentWidget = nullptr;` please remove that and use the init list of the constructor to init the pointer to null. no point creating a qwidget and replacing it again with a nullptr
> > - the connect statements have excess spaces
> > - the connect statement in videowindow.cpp should be changed to use c++11 addresss-of style rather than SIGNAL()
> > - the ifs in stateChange.cpp need spaces before the opening bracket
> 
> Anthony Fieroni wrote:
>     Style is mixing, old used style if( x ), one line statement without brasses {}, does not match new style i prefer to *adjust* patches to that style. About me it's better than mixing it.

https://phabricator.kde.org/D3802


- Anthony


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129695/#review101552
-----------------------------------------------------------


On Дек. 23, 2016, 8:16 преди обяд, Anthony Fieroni wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129695/
> -----------------------------------------------------------
> 
> (Updated Дек. 23, 2016, 8:16 преди обяд)
> 
> 
> Review request for KDE Multimedia and Harald Sitter.
> 
> 
> Repository: dragon
> 
> 
> Description
> -------
> 
> Patch was rejected from reviewboard, see attached file and link to pastebin for pretty format https://paste.kde.org/pm1yzkjdl
> 
> 
> Diffs
> -----
> 
>   src/app/recentlyPlayedList.cpp 2c25e7f 
>   src/app/stateChange.cpp 7bf4038 
> 
> Diff: https://git.reviewboard.kde.org/r/129695/diff/
> 
> 
> Testing
> -------
> 
> Fixed one frame blinking audioview before video starts, it's because information (hasVideo) is not resolved immediately after a media object gets a new source.
> Tested scenarios:
> 
> Play video file -> videoview -> wait to auto end -> loadview
> Play video file -> videoview -> press stop -> loadview
> Play video file -> videoview -> press Play media (preview) -> wait to end -> loadview
> Play video file -> videoview -> press Play media (preview) -> press stop -> loadview
> 
> Play audio file -> audioview -> wait to auto end -> loadview
> Play audio file -> audioview -> press stop -> loadview
> Play audio file -> audioview -> press Play media -> wait to end -> loadview
> Play audio file -> audioview -> press Play media -> press stop -> loadview
> 
> 
> File Attachments
> ----------------
> 
> real patch
>   https://git.reviewboard.kde.org/media/uploaded/files/2016/12/23/87b20eea-30b9-4a6c-8915-bfbc0f4f3cd8__simple.patch
> 
> 
> Thanks,
> 
> Anthony Fieroni
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-multimedia/attachments/20161223/f2826334/attachment.htm>