[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