[KPhotoAlbum] Patch for proper "Open With..."
Jan Kundrát
jkt at gentoo.org
Mon Oct 18 03:32:07 BST 2010
Hi Andreas, thanks for your patch, unfortunately I can't really commit
it, reasons are below.
> + // check for the special entry for self-defined
> + if (name == i18n("Your Command Line")) {
> +
> + static RunDialog* dialog;
> + dialog = new RunDialog(MainWindow::Window::theMainWindow(), _list);
> + dialog->show();
> +
> + return;
> + }
Sorry, but this is just ugly. I haven't checked the code (sorry for
that), but please do the check based on the QAction* action's value and
not the actual string data (that might involve keeping track of the
pointer value's somewhere if the list of QActions is somehow
runtime-generated). I realize that's from the old code, but I still do
not like it at all.
Also note that your code leaks memory, unless there's a proper
deleteLater() connected to the dialog's slots (and if it was, current
code would be broken). Current version won't leak, as the constructor is
called exactly once (the GUI runs in one thread, so that's not an issue
here), while you call it each time it's needed, not destroying the
dialog upon its completion. If you go that way, there's also no need to
define the local variable as static, either.
> - return; //user clicked the title entry. (i.e: "All Selected Items")
> + return; //user clicked the title entry. (i.e: "All Selected Items")
Please do not mix whitespace cleanup/changes with real changes.
Cheers,
-jkt
--
cd /local/pub && more beer > /dev/mouth
More information about the Kphotoalbum
mailing list