Re: [lmi] Failing more gracefully with missing PNG files

[Vadim Zeitlin]

On Tue, 15 Sep 2015 22:56:57 +0000 Greg Chicares <address@hidden> wrote:

GC> icon_monger::CreateBitmap() seems to do something suitable already,

 I'm not really sure if it does, being a wxArtProvider as this implies that
it is used for loading often-used fixed-size images and neither of the
hyphenated words seems to be appropriate in this case.

GC> I can see that this is caused by code in class icon_monger, which assumes a
GC> size of (-1, -1) in this case, but I don't even know what class wxArtClient
GC> is, so I figured I'd better ask for help instead of trying to figure this
GC> out myself.

 I don't think it's possible to define a reasonable wxArtClient for this
case. We probably could make this work somehow, but it would never be
anything other than a hack.

GC> I'm not sure class icon_monger is the right tool for this job;
GC> maybe I should really factor out the part of icon_monger::CreateBitmap()
GC> that tests for file existence (in two places) and loads the PNG image if
GC> a valid one is found, otherwise returning wxNullBitmap.

 Yes, this is exactly what I was going to propose.

GC> This seems to work:
GC> 
GC> #include "data_directory.hpp"           // AddDataDir()
GC> #include <boost/filesystem/operations.hpp>
GC> #include <boost/filesystem/path.hpp>
GC> ...
GC> wxImage load_image(char const* file)
GC> {
GC>     fs::path image_path(file);
GC>     if(!fs::exists(image_path))
GC>         {
GC>         image_path = AddDataDir(file);
GC>         }
GC>     if(!fs::exists(image_path))
GC>         {
GC>         warning()
GC>             << "Unable to find image '"
GC>             << image_path.string()
GC>             << "'. Try reinstalling."
GC>             << "\nA blank image will be used instead."
GC>             << LMI_FLUSH
GC>             ;
GC>         return wxImage();
GC>         }
GC>     wxImage image(image_path.string());
GC>     if(!image.IsOk())
GC>         {
GC>         warning()
GC>             << "Unable to load image '"
GC>             << image_path.string()
GC>             << "'. Try reinstalling."
GC>             << "\nA blank image will be used instead."
GC>             << LMI_FLUSH
GC>             ;
GC>         return wxImage();
GC>         }
GC> 
GC>     return image;
GC> }
GC> 
GC> though the code duplication is regrettable;

 AFAICS the only reason for it, i.e. the only reason for which the code
above can't be reused in icon_monger::CreateBitmap(), is that the error
messages are slightly different. But this could be worked around by having
some do_load_image() returning an error code allowing the caller to give
(or not) the error message on its own. Alternatively, we could keep a
single load_image() but add a "fallback" argument determining if the
missing file should be silently ignored (for lmi-specific icons) or warned
about and, if so, how.

 BTW, I'm not sure why do we give detailed explanations ("a builtin
alternative will be used" or "a blank icon will be used") if the file is
not found but not if it's found but can't be loaded. It looks like it
should be given in either case.

GC> should I just put aside my regrets and commit that,

 I think avoiding code duplication would be preferable. E.g. it would be
nice to add support for "high DPI" images one of these days and it would be
better if the logic for this (looking for images with doubled size either
in a different (sub)directory or by appending "@2x" suffix to their names)
could be contained in a single place only.

GC> or is there a better way?

 I don't see anything radically better.

 Would you like me to propose a patch implementing the above or will you
just finish the code you already have?
VZ