[vlc-devel] [RESEND] [PATCH] Qt: Enable Stop Time in media convert dialog
Filip Roséen
filip at atch.se
Tue Nov 8 11:26:50 CET 2016
Hi Rajeesh,
Keep up the good work!
Even though it might be annoying to receive feedback over and over, I
hope it is constructive enough to not wear you down (I know that I
usually down a trillion cups of coffee when I get into a
review-discussion where I keep fixing and fixing and fixing.. *inf*).
On 2016-11-08 15:41, Rajeesh K V wrote:
> Hello Filip,
>
> On Tue, Nov 8, 2016 at 2:47 PM, Filip Roséen wrote:
>
> > The more editors, the merrier! Yes, the indentation looks correct in the
> > patch you attached, you have however attached the result of running git
> > diff, and not git format-patch (where the latter is what you should use).
>
> Attached the updated patch, generated using `git format-patch'.
Super!
> > The current check might lead to unexpected behavior (from the users
> > perspective). Personally I would assume that if I set the start- and
> > stop-time to the same value, the length would be 0; with your patch the
> > stop-time would simply be discarded.
> >
> > For what it is worth the core seems to suffer from the same problem, not
> > sure if this is documented behavior but if I remember I’ll take a look at it
> > later.
>
> OK.
> I'd imagine if the user casually left stop-time same as start-time it
> probably is an oversight (would someone want a converted file of 0
> length?), and conservatively consider stop-time till the end. But I
> reckon a warning message would help.
Indeed, though you could view the addition of a warning to be an
enhancement that can be implemented in the future (there's no need to
address this issue immediately).
> > If one is to nit-pick the implementation suffer from a relatively minor case
> > of code-duplication, but more importantly; the changed line feels very
> > crammed.
>
> Refactored into a new function.
>
> > Please address the above diagnostic (by making sure that that the name is
> > not the same as for the start-time control, or anything else for that
> > matter).
>
> Mea culpa. Fixed in the new patch.
>
>
> --
> Rajeesh
> From 4e4b7e86cd37ec10f1a73174d7b98fcd88971387 Mon Sep 17 00:00:00 2001
> From: Rajeesh K V <rajeeshknambiar at gmail.com>
> Date: Tue, 16 Aug 2016 10:38:46 +0530
> Subject: [PATCH] Qt: Enable Stop Time in media convert dialog
>
> Convert/Save dialog has had option to specify 'start time' but
> no option for setting 'stop time' in the interface. The backend
> 'stop-time' option was already present. This patch simply adds
> and enables the edit widget for end time in Qt interface.
> ---
> modules/gui/qt/dialogs/open.cpp | 20 ++++++++++++++------
> modules/gui/qt/dialogs/open.hpp | 1 +
> modules/gui/qt/ui/open.ui | 33 +++++++++++++++++++++++++++++++++
> 3 files changed, 48 insertions(+), 6 deletions(-)
>
> diff --git a/modules/gui/qt/dialogs/open.cpp b/modules/gui/qt/dialogs/open.cpp
> index 224cfa5..37a2e09 100644
> --- a/modules/gui/qt/dialogs/open.cpp
> +++ b/modules/gui/qt/dialogs/open.cpp
> @@ -156,6 +156,7 @@ OpenDialog::OpenDialog( QWidget *parent,
> CONNECT( ui.slaveText, textChanged( const QString& ), this, updateMRL() );
> CONNECT( ui.cacheSpinBox, valueChanged( int ), this, updateMRL() );
> CONNECT( ui.startTimeTimeEdit, timeChanged ( const QTime& ), this, updateMRL() );
> + CONNECT( ui.stopTimeTimeEdit, timeChanged ( const QTime& ), this, updateMRL() );
> BUTTONACT( ui.advancedCheckBox, toggleAdvancedPanel() );
> BUTTONACT( ui.slaveBrowseButton, browseInputSlave() );
>
> @@ -179,6 +180,7 @@ OpenDialog::OpenDialog( QWidget *parent,
>
> /* enforce section due to .ui bug */
> ui.startTimeTimeEdit->setCurrentSection( QDateTimeEdit::SecondSection );
> + ui.stopTimeTimeEdit->setCurrentSection( QDateTimeEdit::SecondSection );
>
> setMinimumSize( sizeHint() );
> setMaximumWidth( 900 );
> @@ -436,12 +438,10 @@ void OpenDialog::updateMRL() {
> mrl += QString( " :%1=%2" ).arg( storedMethod ).
> arg( ui.cacheSpinBox->value() );
> if( ui.startTimeTimeEdit->time() != ui.startTimeTimeEdit->minimumTime() ) {
> - mrl += QString( " :start-time=%1.%2" )
> - .arg( QString::number(
> - ui.startTimeTimeEdit->minimumTime().secsTo(
> - ui.startTimeTimeEdit->time()
> - ) ) )
> - .arg( ui.startTimeTimeEdit->time().msec(), 3, 10, QChar('0') );
> + mrl += " :start-time=" + TimeTomSecs( ui.startTimeTimeEdit );
> + }
> + if( ui.stopTimeTimeEdit->time() > ui.startTimeTimeEdit->time() ) {
> + mrl += " :stop-time=" + TimeTomSecs( ui.stopTimeTimeEdit );
> }
> ui.advancedLineInput->setText( mrl );
> ui.mrlLine->setText( itemsMRL.join( " " ) );
> @@ -450,6 +450,14 @@ void OpenDialog::updateMRL() {
> selectButton->setEnabled( !itemsMRL.isEmpty() );
> }
>
> +/* Format the time to millisectionds */
> +QString OpenDialog::TimeTomSecs( const QTimeEdit *time ) {
> + if ( ! time ) {
- Is the above check really necessary?
- Does it ever make sense to call the function with `nullptr`?
- If it is; does it make sense to return an empty string, and not
`"0"` if the function receives `nullptr`?
> + return QString("");
> + }
> + return QString::number( ( time->minimumTime().msecsTo( time->time() ) ) / 1000.0, 'f', 3);
> +}
> +
> /* Change the caching combobox */
> void OpenDialog::newCachingMethod( const QString& method )
> {
> diff --git a/modules/gui/qt/dialogs/open.hpp b/modules/gui/qt/dialogs/open.hpp
> index 0ce6f81..707c4e2 100644
> --- a/modules/gui/qt/dialogs/open.hpp
> +++ b/modules/gui/qt/dialogs/open.hpp
> @@ -102,6 +102,7 @@ private:
> int i_action_flag;
> bool b_pl;
> QStringList SeparateEntries( const QString& );
> + QString TimeTomSecs( const QTimeEdit* );
This function does not have to be a *member-function*. Given that it
is only used in `open.cpp`, and that it does not need access to any
members of `OpenDialog`, a function with *internal-linkage* defined
directly in `open.cpp` would, in my opinion, be cleaner.
**If** you have a reason for it being a *member-function*, the
*member-function* should be `const` qualified (given that it does not
change the internal state of the function).
>
> QPushButton *cancelButton, *selectButton;
> QToolButton *playButton;
> diff --git a/modules/gui/qt/ui/open.ui b/modules/gui/qt/ui/open.ui
> index ea510db..f105fe4 100644
> --- a/modules/gui/qt/ui/open.ui
> +++ b/modules/gui/qt/ui/open.ui
> @@ -169,6 +169,19 @@
> </property>
> </widget>
> </item>
> + <item row="1" column="3">
> + <widget class="QLabel" name="label_4">
> + <property name="text">
> + <string>Stop Time</string>
> + </property>
> + <property name="alignment">
> + <set>Qt::AlignRight|Qt::AlignTrailing|Qt::AlignVCenter</set>
> + </property>
> + <property name="buddy">
> + <cstring>stopTimeTimeEdit</cstring>
> + </property>
> + </widget>
> + </item>
> <item row="8" column="0">
> <widget class="QLabel" name="label">
> <property name="text">
> @@ -241,6 +254,25 @@
> </property>
> </widget>
> </item>
> + <item row="1" column="4" colspan="2">
> + <widget class="QTimeEdit" name="stopTimeTimeEdit">
> + <property name="toolTip">
> + <string>Change the stop time for the media</string>
> + </property>
> + <property name="alignment">
> + <set>Qt::AlignRight|Qt::AlignTrailing|Qt::AlignVCenter</set>
> + </property>
> + <property name="currentSection">
> + <enum>QDateTimeEdit::HourSection</enum>
> + </property>
> + <property name="displayFormat">
> + <string>HH'H':mm'm':ss's'.zzz</string>
> + </property>
> + <property name="timeSpec">
> + <enum>Qt::LocalTime</enum>
> + </property>
> + </widget>
> + </item>
> <item row="4" column="0" colspan="6">
> <widget class="QCheckBox" name="slaveCheckbox">
> <property name="text">
> @@ -271,6 +303,7 @@
> <tabstop>advancedCheckBox</tabstop>
> <tabstop>cacheSpinBox</tabstop>
> <tabstop>startTimeTimeEdit</tabstop>
> + <tabstop>stopTimeTimeEdit</tabstop>
> <tabstop>slaveCheckbox</tabstop>
> <tabstop>slaveText</tabstop>
> <tabstop>slaveBrowseButton</tabstop>
> --
> 2.7.4
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20161108/0a4e3e3b/attachment.html>
More information about the vlc-devel
mailing list