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

[Rajeesh K V]

Hi Filip,

On Tue, Nov 8, 2016 at 3:56 PM, Filip Roséen  wrote:
> 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).

No, I really appreciate the objective feedback. It cuts both ways,
thanks for taking time to review and share the feedback.


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

OK.

>  +/* 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?

No, since the parameter is a pointer, I just took a conservative path.
As this is a private function, removed the spurious check.

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

I concur. Changed it to standalone function.
Please see attached 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: 4941 bytes
Desc: not available
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20161108/4eef67d3/attachment.bin>