Merge lp:~seb128/sessioninstaller/handle-parsing-errors into lp:sessioninstaller

[Sebastien Bacher]

try to handle invalid desktops better (lp: #1190097)

try to handle invalid desktops better (lp: #1190097)

[Sebastien Bacher (seb128)]

That seems to fix the issue described on lp: #1190097.

Not sure if it would be better to restrict the except to ParsingError or similar, but when I tried to limit the type things didn't work as they should for some reason...

[Martin Pitt (pitti)]

Catching *all* exceptions is dangerous: You'd also silently ignore SyntaxError, AttributeError, NameError and the like, and could very easily break that code completely.

So catching DesktopEntry.ParsingError specifically sounds more appropriate. Why does that not work?

review: Needs Fixing

[Sebastien Bacher (seb128)]

I don't know, I'm probably overlooking something easy but the exception is

" File "/usr/lib/python2.7/dist-packages/xdg/IniFile.py", line 81, in parse
    raise ParsingError("Invalid line: " + line, filename)
ParsingError: ParsingError in file '/usr/share/app-install/desktop/workrave:workrave.desktop'"

I tried to "except ParsingError:" or "except DesktopEntry.ParsingError:" but the code doesn't enter the except branch when doing that...

[lp:~seb128/sessioninstaller/handle-parsing-errors]

140. By Sebastien Bacher on 2013-06-13

only ignore parsing errors

[Sebastien Bacher (seb128)]

thanks pitti for helping figuring the namespace issue, pushed an updated version that only catch parsing errors

[Martin Pitt (pitti)]

Thanks!

review: Approve

[Michael Vogt (mvo)]

On Wed, Jun 12, 2013 at 02:56:30PM -0000, Sebastien Bacher wrote:
> Sebastien Bacher has proposed merging lp:~seb128/sessioninstaller/handle-parsing-errors into lp:sessioninstaller.
>
> Commit message:
> try to handle invalid desktops better (lp: #1190097)
>
> Requested reviews:
> Aptdaemon Developers (aptdaemon-developers)
> Related bugs:
> Bug #1190097 in nautilus (Ubuntu): "Nautilus tries but fails to auto-install applications needed to open files of a given type"
> https://bugs.launchpad.net/ubuntu/+source/nautilus/+bug/1190097
>
> For more details, see:
> https://code.launchpad.net/~seb128/sessioninstaller/handle-parsing-errors/+merge/168970
>
> try to handle invalid desktops better (lp: #1190097)

Thanks Seb!

> === modified file 'sessioninstaller/core.py'
> --- sessioninstaller/core.py 2013-03-05 05:52:47 +0000
> +++ sessioninstaller/core.py 2013-06-12 14:53:30 +0000
> @@ -1026,8 +1026,11 @@
> progress.hide()
> progress.destroy()
> raise errors.ModifyCancelled
> - desktop_entry = DesktopEntry(os.path.join(utils.APP_INSTALL_DATA,
> + try:
> + desktop_entry = DesktopEntry(os.path.join(utils.APP_INSTALL_DATA,
> path))
> + except:
> + continue
[..]

One tiny nitpick, in this case "pass" is more probably a better choice
than "continue" as the later is mostly used with loops.

And another one: The except is very broad, something like:
from xdg.Exception import Error as XdgError
  try:...
  except XdgError: pass

has the advantage that e.g. typos in the line (when refactoring) will
not be silently swallowed.

Tiny things, patch is great and a welcome addition. I can merge once I
get better internet (on the train right now).

Cheers,
 Michael

=== modified file 'sessioninstaller/core.py'
--- sessioninstaller/core.py	2013-03-05 05:52:47 +0000
+++ sessioninstaller/core.py	2013-06-13 09:38:31 +0000
@@ -48,6 +48,7 @@
 from gi.repository import Gtk
 from gi.repository import Pango
 from xdg.DesktopEntry import DesktopEntry
+from xdg.Exceptions import ParsingError
 
 import utils
 import errors
@@ -1026,8 +1027,11 @@
                     progress.hide()
                     progress.destroy()
                     raise errors.ModifyCancelled
-            desktop_entry = DesktopEntry(os.path.join(utils.APP_INSTALL_DATA,
+            try:
+                desktop_entry = DesktopEntry(os.path.join(utils.APP_INSTALL_DATA,
                                                       path))
+            except ParsingError:
+                continue
             pkg_name = desktop_entry.get("X-AppInstall-Package")
             try:
                 if self._cache[pkg_name].is_installed: