[vlc-devel] [RESEND] [PATCH] Qt: Enable Stop Time in media convert dialog
Filip Roséen
filip at atch.se
Tue Nov 8 10:17:30 CET 2016
Hi Rajeesh,
On 2016-11-08 08:37, Rajeesh K V wrote:
> The email client did the line wrapping (used Geary as gmail web interface is known to
> wrap lines, but that didn't help either).
> Addressing your review comments, attached the updated patch. I checked the indentation
> in 4 different editors/apps, hopefully it is correct.
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).
> commit 139c10ec90793376e2d4621cd787701b79681349
> Author: Rajeesh K V <rajeeshknambiar at gmail.com>
> Date: Tue Aug 16 10:38:46 2016 +0530
>
> 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.
>
> diff --git a/modules/gui/qt/dialogs/open.cpp b/modules/gui/qt/dialogs/open.cpp
> index 224cfa5..6457a63 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,12 @@ 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=" + QString::number((ui.startTimeTimeEdit->minimumTime().
> + msecsTo(ui.startTimeTimeEdit->time() ) )/1000.0,'f',3);
> + }
> + if( ui.stopTimeTimeEdit->time() > ui.startTimeTimeEdit->time() ) {
If the *start-* and *stop-time* are the same (but not `0`), it might
make more sense to either display an error message (or signal it in
some other way), or simply pass the same values for both in the `mrl`
(ie. trust the user).
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.
> + mrl += " :stop-time=" + QString::number((ui.stopTimeTimeEdit->minimumTime().
> + msecsTo(ui.stopTimeTimeEdit->time() ) )/1000.0,'f',3);
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.
> }
> ui.advancedLineInput->setText( mrl );
> ui.mrlLine->setText( itemsMRL.join( " " ) );
> diff --git a/modules/gui/qt/ui/open.ui b/modules/gui/qt/ui/open.ui
> index ea510db..05d8193 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_3">
UIC ui/open.h
modules/gui/qt/ui/open.ui: Warning: The name 'label_3' (QLabel) is already in use, defaulting to 'label_31'
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).
> + <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>
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20161108/c5363d82/attachment.html>
More information about the vlc-devel
mailing list