<!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>Keep up the good work!</p>
<p>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.. <em>inf</em>).</p>
<p>On 2016-11-08 15:41, Rajeesh K V wrote:</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> Hello Filip,
On Tue, Nov 8, 2016 at 2:47 PM, Filip Roséen wrote:</code></pre>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> 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).</code></pre>
</blockquote>
<pre><code> Attached the updated patch, generated using `git format-patch'.</code></pre>
</blockquote>
<p>Super!</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> 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.</code></pre>
</blockquote>
<pre><code> 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.</code></pre>
</blockquote>
<p>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).</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> 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.</code></pre>
</blockquote>
<pre><code> Refactored into a new function.</code></pre>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> 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).</code></pre>
</blockquote>
<pre><code> Mea culpa. Fixed in the new patch.
--
Rajeesh</code></pre>
</blockquote>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> From 4e4b7e86cd37ec10f1a73174d7b98fcd88971387 Mon Sep 17 00:00:00 2001
From: Rajeesh K V <rajeeshknambiar@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 ) {</code></pre>
</blockquote>
<ul>
<li>Is the above check really necessary?</li>
<li>Does it ever make sense to call the function with <code>nullptr</code>?</li>
<li>If it is; does it make sense to return an empty string, and not <code>"0"</code> if the function receives <code>nullptr</code>?</li>
</ul>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> + 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* );</code></pre>
</blockquote>
<p>This function does not have to be a <em>member-function</em>. Given that it is only used in <code>open.cpp</code>, and that it does not need access to any members of <code>OpenDialog</code>, a function with <em>internal-linkage</em> defined directly in <code>open.cpp</code> would, in my opinion, be cleaner.</p>
<p><strong>If</strong> you have a reason for it being a <em>member-function</em>, the <em>member-function</em> should be <code>const</code> qualified (given that it does not change the internal state of the function).</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> 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
</code></pre>
</blockquote>
</body>
</html>