<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
  <meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
  <meta http-equiv="Content-Style-Type" content="text/css" />
  <meta name="generator" content="pandoc" />
  <title></title>
  <style type="text/css">code{white-space: pre;}</style>
</head>
<body>
<p>Hi Rajeesh,</p>
<p>On 2016-11-08 08:37, Rajeesh K V wrote:</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> 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.</code></pre>
</blockquote>
<p>The more editors, the merrier! Yes, the indentation looks correct in the patch you attached, you have however attached the result of running <code>git diff</code>, and not <code>git format-patch</code> (where the latter is what you should use).</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> commit 139c10ec90793376e2d4621cd787701b79681349
 Author: Rajeesh K V <rajeeshknambiar@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() ) {</code></pre>
</blockquote>
<p>If the <em>start-</em> and <em>stop-time</em> are the same (but not <code>0</code>), 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 <code>mrl</code> (ie. trust the user).</p>
<p>The current check might lead to unexpected behavior (from the users perspective). Personally I would assume that if I set the <em>start-</em> and <em>stop-time</em> to the same value, the length would be <code>0</code>; with your patch the <em>stop-time</em> would simply be discarded.</p>
<p>For what it is worth the <em>core</em> 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.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> +        mrl += " :stop-time=" + QString::number((ui.stopTimeTimeEdit->minimumTime().
 +                                msecsTo(ui.stopTimeTimeEdit->time() ) )/1000.0,'f',3);</code></pre>
</blockquote>
<p>If one is to <em>nit-pick</em> the implementation suffer from a relatively minor case of code-duplication, but more importantly; the changed line feels very crammed.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code>      }
      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"></code></pre>
</blockquote>
<pre><code>UIC     ui/open.h
modules/gui/qt/ui/open.ui: Warning: The name 'label_3' (QLabel) is already in use, defaulting to 'label_31'</code></pre>
<p>Please address the above diagnostic (by making sure that that the name is not the same as for the <em>start-time</em> control, or anything else for that matter).</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> +        <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></code></pre>
</blockquote>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> _______________________________________________
 vlc-devel mailing list
 To unsubscribe or modify your subscription options:
 https://mailman.videolan.org/listinfo/vlc-devel</code></pre>
</blockquote>
</body>
</html>