[vlc-devel] [RESEND] [PATCH] Qt: Enable Stop Time in media convert dialog

Rajeesh K V rajeeshknambiar at gmail.com
Tue Nov 8 11:11:33 CET 2016


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'.

>
> 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.

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.

>
>  +        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.

Refactored into a new function.

>  --- 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).

Mea culpa. Fixed in the new patch.


-- 
Rajeesh
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Qt-Enable-Stop-Time-in-media-convert-dialog.patch
Type: text/x-patch
Size: 5477 bytes
Desc: not available
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20161108/af4a9125/attachment.bin>


More information about the vlc-devel mailing list