Review Request 120171: Add option to allow execution as well as opening of scripts and desktop files

[Emmanuel Pescosta]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120171/#review66630
-----------------------------------------------------------



dolphin/src/dolphinmainwindow.h
<https://git.reviewboard.kde.org/r/120171/#comment46493>

    Description please



dolphin/src/dolphinmainwindow.cpp
<https://git.reviewboard.kde.org/r/120171/#comment46441>

    Remove the empty line



dolphin/src/dolphinmainwindow.cpp
<https://git.reviewboard.kde.org/r/120171/#comment46440>

    Just remove the KRun pointer "run" if you don't need it.



dolphin/src/dolphinmainwindow.cpp
<https://git.reviewboard.kde.org/r/120171/#comment46485>

    switch () {
    case Open: {
        ...
        break;
    }
    case AlwaysAsk:
        ...;
        break;
        
    case Execute:
        ...;
        break;
    }



dolphin/src/dolphinmainwindow.cpp
<https://git.reviewboard.kde.org/r/120171/#comment46442>

    Remove the unused KRun pointer + curly brackets



dolphin/src/dolphinmainwindow.cpp
<https://git.reviewboard.kde.org/r/120171/#comment46487>

    Maybe you want a scoped pointer instead? See http://qt-project.org/doc/qt-5/qscopedpointer.html
    
    If you use the scoped pointer you can remove the "delete dialog;" at the end of this method



dolphin/src/dolphinmainwindow.cpp
<https://git.reviewboard.kde.org/r/120171/#comment46488>

    .data() is not needed



dolphin/src/dolphinmainwindow.cpp
<https://git.reviewboard.kde.org/r/120171/#comment46486>

    switch () {
    case Open: {
        ...
        break;
    }
    case AlwaysAsk:
        ...;
        break;
        
    case Execute:
        ...;
        break;
    }



dolphin/src/dolphinmainwindow.cpp
<https://git.reviewboard.kde.org/r/120171/#comment46490>

    Remove unused KRun* run



dolphin/src/dolphinviewcontainer.h
<https://git.reviewboard.kde.org/r/120171/#comment46437>

    I think we should name this signal "fileActivated" instead of "fileClicked".



dolphin/src/settings/general/behaviorsettingspage.h
<https://git.reviewboard.kde.org/r/120171/#comment46492>

    Give them better names please.



dolphin/src/settings/general/behaviorsettingspage.cpp
<https://git.reviewboard.kde.org/r/120171/#comment46451>

    only 1 space between



dolphin/src/settings/general/behaviorsettingspage.cpp
<https://git.reviewboard.kde.org/r/120171/#comment46450>

    revert the unrelated new line change



dolphin/src/settings/general/behaviorsettingspage.cpp
<https://git.reviewboard.kde.org/r/120171/#comment46452>

    Remove empty new line



dolphin/src/views/executablefileopendialog.h
<https://git.reviewboard.kde.org/r/120171/#comment46449>

    m_ prefix



dolphin/src/views/executablefileopendialog.cpp
<https://git.reviewboard.kde.org/r/120171/#comment46447>

    Can be removed



dolphin/src/views/executablefileopendialog.cpp
<https://git.reviewboard.kde.org/r/120171/#comment46448>

    Can be removed


- Emmanuel Pescosta


On Sept. 16, 2014, 12:12 a.m., Arjun Ak wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/120171/
> -----------------------------------------------------------
> 
> (Updated Sept. 16, 2014, 12:12 a.m.)
> 
> 
> Review request for Dolphin, KDE Usability and David Faure.
> 
> 
> Bugs: 275405
>     http://bugs.kde.org/show_bug.cgi?id=275405
> 
> 
> Repository: kde-baseapps
> 
> 
> Description
> -------
> 
> This patch allows the user to execute (in background, within the embedded terminal, external terminal) or open an executable file (scripts, desktop files...). When the user clicks on an executable file, a [dialog](https://git.reviewboard.kde.org/media/uploaded/files/2014/09/12/0fa53887-666f-4121-9ec6-9f323d633d03__dialog.png) pops up which contains options to either open it or execute it. Clicking on execute runs the program in the background (dolphin's current behaviour).
> 
> The [settings dialog](https://git.reviewboard.kde.org/media/uploaded/files/2014/09/12/cd4d14d3-dd82-4c2c-8c0a-c2a428c78903__settings.png) has further options to fine-tune how the file is executed. The program can either be executed in the background or inside the embedded konsole or in an external terminal.
> 
> In case there is a process already running in the embedded terminal, with [user's permission](https://git.reviewboard.kde.org/media/uploaded/files/2014/09/12/c5d7843a-099a-43d4-a2e1-6ce429623866__replace.png) it will be terminated and the selected program will be started.
> 
> 
> See also:
> https://git.reviewboard.kde.org/r/118939/
> 
> 
> Diffs
> -----
> 
>   dolphin/src/views/executablefileopendialog.cpp PRE-CREATION 
>   dolphin/src/dolphinviewcontainer.cpp a11ba42 
>   dolphin/src/settings/dolphin_generalsettings.kcfg 849a9c7 
>   dolphin/src/settings/general/behaviorsettingspage.h 7a9c2f0 
>   dolphin/src/settings/general/behaviorsettingspage.cpp cbbde1d 
>   dolphin/src/views/executablefileopendialog.h PRE-CREATION 
>   dolphin/src/CMakeLists.txt 6f256a2 
>   dolphin/src/dolphinmainwindow.h 9d4c003 
>   dolphin/src/dolphinmainwindow.cpp 95b08af 
>   dolphin/src/dolphinviewcontainer.h 31612f1 
> 
> Diff: https://git.reviewboard.kde.org/r/120171/diff/
> 
> 
> Testing
> -------
> 
> 
> File Attachments
> ----------------
> 
> Dialog
>   https://git.reviewboard.kde.org/media/uploaded/files/2014/09/12/0fa53887-666f-4121-9ec6-9f323d633d03__dialog.png
> settings
>   https://git.reviewboard.kde.org/media/uploaded/files/2014/09/12/cd4d14d3-dd82-4c2c-8c0a-c2a428c78903__settings.png
> Replace current program
>   https://git.reviewboard.kde.org/media/uploaded/files/2014/09/12/c5d7843a-099a-43d4-a2e1-6ce429623866__replace.png
> 
> 
> Thanks,
> 
> Arjun Ak
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20140916/8beeb6c7/attachment.htm>