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

[Greg Chicares]

On 2015-09-16 00:45, Vadim Zeitlin wrote:
> 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.

Thanks, that's what I failed to understand.

> 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:

[...snip code that doesn't "factor out" that part, but instead duplicates it,
more or less...]

>  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.

There are superficial differences like
  "Unable to find icon  ... [icon_monger CreateBitmap()]
  "Unable to find image ... [group quotes load_image()]
                  ^^^^^
but there are deeper differences as well. One function returns a wxImage
(and a default-constructed wxImage on error) while the other returns a
wxBitmap constructed from a valid image if available, or wxNullBitmap
otherwise...and, while I didn't go back and retest it to make sure, I did
get the impression that wxNullBitmap.ConvertToImage() elicits a warning
from wx. Furthermore, icon_monger::CreateBitmap() has the specialized
notion of "built-in" icons, which differs by platform (so that wxGTK is
"themable"...oh, wait...no, "themability" and "built-in-ness" are two
distinct ideas); and it also has resizing logic that's more appropriate
for icons than for the group-quote images (which we might want to resize,
but in a way that doesn't use icon_monger's wxArtClient-dependent function
desired_icon_size()).

> 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.

I spent a few minutes trying to do that, but I wasn't satisfied with the
result. In light of the real differences described above, factoring out
what's more or less common made the code harder to understand.

>  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.

That does look like an unintentional omission. I'll add a diagnostic there,
because it doesn't seem possible for it to cause harm. I don't think it's
possible to reach this new diagnostic on msw, though, where all icons are
explicitly given (i.e., they're all elements of lmi_specific_icon_names_)
and none are themable...so I guess the (former) lack of a diagnostic was a
real lmi-GTK problem.

> 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.

I'd rather consider high-DPI alternative images only at some future time
when we potentially decide we want them. Right now, this tangential matter
has grabbed too much of my interest, and I need to pry myself away in order
to focus on the Halloween release, for which we still don't have final
specifications yet.

> 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?

I'm going to commit what I have. If you disagree with my analysis above
and still want to propose a patch to refactor this, then I'll consider
it, but I have to say I'm prejudiced against it. It took me some effort
to decide that icon_monger::CreateBitmap()'s tests (with diagnostics)
occur in the right order (I think they do). Factoring out some (but not
all) of those tests into a separate function will, I fear, make the
order of those tests harder to understand. And if we have to write a
function template to abstract out the wxImage vs. wxBitmap datatype,
that would just add one more layer of difficulty.

Still, it's a pity to duplicate a sizable chunk of code.