Review Request: TaskManager: Prompt user when automatic task to launcher mathing fails.

Wed Nov 2 13:57:42 UTC 2011

> On Nov. 1, 2011, 3:07 p.m., Aaron J. Seigo wrote:
> > the wholesale copying of KOpenWithDialog is very unfortunate and needs to be avoided if at *all* possible. why is it copied instead of used directly?
> > 
> > the really big issue is the exec()'ing of the dialog, however.
> > 
> > it would also be nice to be able to access individual launcher configuration via the individual launchers' context menu.
> > 
> > p.s. whitespace :)
> 
> Craig Drummond wrote:
>     I agree the copying is bad - but without changing kdelibs I knew of no other way. The reason it is copied is that in KOpenWithDialog you cannot remove the "Open with" label (or change its text), or the kurlrequester below it.

the Open With label is settable by using the KOpenWithDialog( const KUrl::List& urls, const QString& text, const QString& value, QWidget *parent = 0 ); constructor, so it could be set to something sensible for this case. as forthe KUrlRequester ... is that so bad? it allows the user to define an arbitrary exec on disk, the ultimate "last resort".

> On Nov. 1, 2011, 3:07 p.m., Aaron J. Seigo wrote:
> > libs/taskmanager/taskitem.cpp, line 362
> > <http://git.reviewboard.kde.org/r/103007/diff/1/?file=39957#file39957line362>
> >
> >     in which cases would the mapping not be saved (e.g. saveMapping == false)?
> 
> Craig Drummond wrote:
>     In another patch (not done yet), LauncherItem::associateItemIfMatches() calls setLauncherUrl on the task. But as this was not done by the user, I didnt want to save the mapping.

ah! well, this could be accomplished with a LauncherItem::setLauncherUrl(LauncherItem *item) overload. code duplication would be minimal (just the first 4 lines of code, really) and the API would be clearer imo.

> On Nov. 1, 2011, 3:07 p.m., Aaron J. Seigo wrote:
> > libs/taskmanager/taskactions.cpp, line 375
> > <http://git.reviewboard.kde.org/r/103007/diff/1/?file=39955#file39955line375>
> >
> >     dialogs must not be exec()'d as they *freeze* the entire UI.. in this case the whole plasma-desktop shell. i must be handled asynchronously.
> 
> Craig Drummond wrote:
>     Good point!
> 
> Craig Drummond wrote:
>     This is odd. I've tried running the dialog via "show()" and connecting to the "okClicked()" signal. However, the slot never gets called. If I change it to "exec()" the slot does get called...
>     
>     e.g. this does *not* work:
>     
>             KApplicationSelectorDialog *dlg=new KApplicationSelectorDialog(i18n("XXX"), 0L);
>             connect(dlg, SIGNAL(okClicked()), this, SLOT(xxx()));
>             dlg->show();
>     
>     ...but this does:
>     
>             KApplicationSelectorDialog *dlg=new KApplicationSelectorDialog(i18n("XXX"), 0L);
>             connect(dlg, SIGNAL(okClicked()), this, SLOT(xxx()));
>             dlg->exec();
>     
>     (Ignore the obvious memory leak)
>     
>     -----------------
>     
>     However, this seems to be a general issue with plasma dialogs. e.g. if you configure a colour for the clock dialog, plasma is frozen. Perhaps all plasma dialogs need to be run via another process?

"e.g. this does *not* work"

that is very, very odd. it would seem to imply that there is an event loop issue. there is no difference otherwise between exec() and show() (in fact, in fixing kcolorbutton i use show()). 

ah! perhaps i see the issue: LauncherProperties::run calls exec() on itself .. that creates an inner event loop around LauncherProperties. my guess is that trips it up. neither LauncherProperties nor KApplicationSelectorDialog should be exec()'d

"(Ignore the obvious memory leak)"

btw, easiest way to deal with this is to call dlg->setAttribute(Qt::WA_DeleteOnClose);

"this seems to be a general issue with plasma dialog"

we have this issue in very few places, and where it remains it is fixable ..  i just fixed KColorButton right now in fact (thanks for pointing that one out)

- Aaron J.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/103007/#review7801
-----------------------------------------------------------

On Oct. 31, 2011, 8:42 p.m., Craig Drummond wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/103007/
> -----------------------------------------------------------
> 
> (Updated Oct. 31, 2011, 8:42 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Description
> -------
> 
> 1. If fail to automatically find launcher, then prompt user to select from installed applications.
> 2. Add a config page, so that manualy set launchers may be adjusted.
> 
> (Part of IconTasks' taskmanager changes)
> 
> 
> Diffs
> -----
> 
>   libs/taskmanager/CMakeLists.txt 57f5f73 
>   libs/taskmanager/groupmanager.h acaa142 
>   libs/taskmanager/groupmanager.cpp 6e7ffa7 
>   libs/taskmanager/kapplicationselectordialog.h PRE-CREATION 
>   libs/taskmanager/kapplicationselectordialog.cpp PRE-CREATION 
>   libs/taskmanager/launcherconfig.h PRE-CREATION 
>   libs/taskmanager/launcherconfig.cpp PRE-CREATION 
>   libs/taskmanager/launcherconfig.ui PRE-CREATION 
>   libs/taskmanager/launcherproperties.h PRE-CREATION 
>   libs/taskmanager/launcherproperties.cpp PRE-CREATION 
>   libs/taskmanager/launcherproperties.ui PRE-CREATION 
>   libs/taskmanager/taskactions.cpp 0e6ba8e 
>   libs/taskmanager/taskitem.h 5de8478 
>   libs/taskmanager/taskitem.cpp 0a768e5 
> 
> Diff: http://git.reviewboard.kde.org/r/103007/diff/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Craig Drummond
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20111102/b385362a/attachment-0001.html>