RE: [PATCH]bug 44516 improved label and business card document creation

Norbert Thiebaud wrote (25 februari 2012 11:26)

> ...
> why is P78275 removed ?
> ...
> same question for these 2
> ...
> and these 3 etc...

I have have removed some obsolete labels. I even removed one complete
brand (Herlitz) after communication with the manufacturer. Herlitz
labels are no longer produced and no longer on the market.

> so... If these changes are intentional I would urge you to:
> + do the cosmetic white-space/indentation fixes to label.xcu in a
> separate patch that contain only these (i.e diff -u -w -> empty diff)
> + do the needed removal addition of labels in a separate patch,
> preferably with some explanations in the commit message as to why
> + do the wholesale addition of the 2 new attributes (as I understand
> adding these attributes does not _require_ the underlying code change
> right ?
> + then a patch with he code change
> That would render it possible for someone to really review the changes

I understand what you mean. It started with 'just' adding 2 values (i.e.
page width and page height). To add these, I had to verify the paper 
dimensions when the calculated dimensions were not exactly A4, A5, 
Letter. Whilst verifying I found sevral obsolete 9and unverify-able
labels and removed them. The diff file produced by these changes was
that large, that I added the lay-out changes, which didn't really make
the diff file much bigger. So I focussed on size of the diff file and 
not on readability/ease of review. 

> #define RC_LABFMT_BEGIN  (RC_ENVELP_BEGIN + 50)
> -#define RC_LABFMT_END    (RC_ENVELP_BEGIN + 59)
> +#define RC_LABFMT_END    (RC_ENVELP_BEGIN + 62)
> ...
> why the shuffling of constants here ?

These constants give the ranges for UI-constants (controls, text 
labels, etc.). I have added two text labels and two input fields to
the label dimensions tab in the label wizard dialog. These fout did
not fit in the range provided, so I had to enlarge the range, with the 
consequence that all ranges after LABFMT had to be moved as well.


> -    SetFillColor(rWinColor);
> +    SetFillColor( Color( 0xE0, 0xE0, 0xFF ) );
> Why the magic numbers for the fill color ?
This colour is used for the label in the graphic representation of the
label with its dimensions. Originally it was an image in shades of gray
(background, paper, label) and I wanted the label to come out better.
Seeing no inconspicous standard colour, I defined one myself. 
With hindsight it would probably have been better to make a constant for
this colour. It is not good practice to leave such colour definitions in 
the code.


I hope my explanations will help you. I not, please say so.

Winfried