iconhandler: Get theme contents from the .deb file, not Contents

[iainlane]

How about doing something like this?

I fixed adwaita-icon-theme to include more icons, but we aren't getting them currently because Contents is updated only once per week on Launchpad. If we look inside the theme's debs directly instead of using Contents then we get its changes right away.

[iainlane]

Ah right, pkg contains this, was looking in Theme. Can use that then

Yeah, that breaks as you observed before. You have to close it before multiprocessing gets involved; you can't pass apt_inst.DebFile (probably actually TarFile) objects across multiprocessing it seems.

Not that big a deal given that we only use a handful of debs here anyway. I now close the deb after looking at it during contents extraction. (Renamed Package.close_file to close so that I can use with closing.)

[iainlane]

Oh you actually supported allowing any package to put files in usr/share/icons/seeded_theme/ for any seeded theme, hicolor isn't special there. OK so I'll add an fnmatch for all the seeded themes shipping files in usr/share/icons and add those first, then overwrite with the package data. That doesn't sound to me worse than what we have currently. Right?

BTW in my test run (Ubuntu main) this didn't increase the number of icon-not-found errors versus running before the patch.

[ximion]

That's likely because your Humanity theme has such a large amount of icons and likely covers all apps which are in main ;-) Things should look differently when run against universe. The approach you mention seems sane, unless fnmatch slows things down a lot (it shouldn't do that though, fnmatch is at least faster than a regex, if I remember correctly).

[iainlane]

OK try now. I'm using a regex now, internets thinks this is faster than fnmatch and fnmatch doesn't support {} expansions anyway, which I wanted to use. :(

[ximion]

Why are you going through the regex hell at all? A smaller patch like

diff --git a/dep11/iconhandler.py b/dep11/iconhandler.py
index 5592e08..d88de7f 100644
--- a/dep11/iconhandler.py
+++ b/dep11/iconhandler.py
@@ -25,6 +25,7 @@ import gi
 gi.require_version('Rsvg', '2.0')
 from gi.repository import Rsvg
 from configparser import ConfigParser
+from contextlib import closing
 from PIL import Image
 from io import StringIO, BytesIO

@@ -148,6 +149,12 @@ class IconHandler:
             for name in self._theme_names:
                 if fname == 'usr/share/icons/{}/index.theme'.format(name):
                     self._themes.append(Theme(name, pkg.filename))
+                    # don't rely just on the Contents file for getting theme icon information, since the
+                    # theme might be updated more frequently than the Contents file (happens on Ubuntu)
+                    with closing(pkg) as p:
+                        for deb_fname in p.debfile.get_filelist():
+                            if deb_fname.startswith('usr/share/icons') and not deb_fname.endswith('/'):
+                                self._icon_files[deb_fname] = pkg
                 elif fname.startswith('usr/share/icons/{}'.format(name)):
                     self._icon_files[fname] = pkg

Should be sufficient - or am I missing something here?

[iainlane]

You're pushing back too much, and I have spent too much time on this already.

[ximion]

No reason to give up... But:

• This feature fixes a small corner case (theme updates quickly) at the expense of opening all theme debfiles everytime the generator is started
• Loading DebFiles and getting their contents is super-expensive, it's the thing the generator spends most time on (I profiled that recently).
• We must support the hicolor special-casing, previous patches broke that (I tested that on Debian)

The diff above does exactly that - I will run with it in production, and maybe make this a config file setting later, since I see this feature might be useful for PPAs which update their theme package quickly. I can disable it on Debian then and save a bit of time, since we have recent Contents files. Should make everyone happy, I think.

[iainlane]

If you don't like the feature then don't accept the pull request - instead you've made me go through iterations rewriting it and still complain and don't really think that it's useful at all.

I don't like how you loop over every theme name for each file in Contents, so that's why I eliminated this by using regular expressions. I don't really find that hellish. Now we have up to two regular expression matches per file versus looping over n theme names as the current code does.

I'm sorry if I broke the hicolor (actually you support this for every theme, hicolor isn't special) before, but that is what asking for reviews is about.

As the maintainer the right to merge code is with you.