Re: [Libreoffice] [PATCH] More icons removal

Hi Michael, Regina,

(jumping in late, so trying to reply to all your concerns)

>        So - just one quick query; if you check:
>
>        vcl/unx/generic/app/soicon.cxx
>
>        you'll see that we calculate the resource id at run-time (this is a bit
> of a gotcha):

[snipped]

>        Did you consider whether it is possible to generate the same number as
> SV_ICON_ID_PRESENTATION_TEMPLATE in some other way ?

I did not as I completely missed this piece of code. Following
Regina's line of thoughts, it looks like this was a mistake. I would
not have much time to look into whether I have made a major mistake
here before a week, you can just revert the changes and I will follow
up on those.

> presumably it is
> not ( in a sane world ), and you removed it from the header which looks
> good. There can be -some- cases like this where a compile would succeed,
> that might fail at runtime - sometimes instead of using ImageLists in a
> pleasant way, this sort of arithmetic happens ;-)

I understand the logic. What would be great is to start unifying the
macros and avoid such magical arithmetic so that we can rely on the
compiler for the checks (unless I am missing something). I am
volunteering to do that if it makes sense!

>        In some ways, I'm surprised to see:
>
> Removed SV_ICON_ID_PRESENTATION_COMPRESSED and all associated code.
>
>        Given that this (apparently) has an 'industrial' version, and we tried
> to focus on icons that were actually visible for that, but - it also
> seems correct :-) (in general it's worth being a bit more paranoid about
> removing them).

I have likely err'd on the side of removing used icons due to the mistake above.

>        Out of interest, how many more unused icons have you detected ? and/or
> do you have a nice script for that ? :-)

I have build a list of #define never used in one file as a beginning
and I am walking through it one by one, checking for references on the
associated icons and the #define values. The list is small (around 10)
and I preferred to do it manually and really looking at the code so no
script so far .I don't know yet how many icons will end up unused.

One I get the hang of it, I may end up with a script to automate that :-)

> I personally would not mind, if these entries in the list have no icons at
> all. But it should be tested, that using this menu item does not result in
> an error.

I would love to add more testing but would need some pointer on how to
add such test, what type of tests (unit test? more like integration
testing in smoke tests?) and where to put such tests.

Thanks,
Julien