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

Winfried Donkers W.Donkers at dci-electronics.nl
Sun Feb 26 23:10:45 PST 2012


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




More information about the LibreOffice mailing list