Bug 18541

Summary: Patron card creator: Add a grid to support layout design
Product: Koha Reporter: Marc Véron <veron>
Component: Label/patron card printingAssignee: Marc Véron <veron>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: alexbuckley, fridolin.somers, katrin.fischer, m.de.rooy
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 18465    
Bug Blocks: 18550    
Attachments: Bug 18541 - Patron card creator: Add a grid to support layout design
Bug 18541 - Patron card creator: Add a grid to support layout design
Bug 18541 - Patron card creator: Add a grid to support layout design
Bug 18541 - Patron card creator: Add a grid to support layout design
Bug 18541 - Patron card creator: Add a grid to support layout design
Unit description and card dimension on patron card batch
Bug 18541 - Patron card creator: Add a grid to support layout design
Bug 18541: (QA follow-up) for comments #11 and #13
Bug 18541 - Patron card creator: Add a grid to support layout design
Bug 18541: (QA follow-up) for comments #11 and #13
Bug 18541: [QA Follow-up] Add POD to Patroncard.pm
Bug 18541: (QA follow-up) for comment #24 (POD)
Bug 18541: (QA follow-up) for comment #24 (POD)

Description Marc Véron 2017-05-04 15:29:51 UTC
In patron card creator, the layout tool has already a guide box that shows the position of a card on the page. The aim of this enhancement is to support the design process with an additonal layout grid to figure out the positions of text fields, barcode and images.
Comment 1 Marc Véron 2017-05-04 15:38:54 UTC Comment hidden (obsolete)
Comment 2 Marc Véron 2017-05-04 15:50:02 UTC Comment hidden (obsolete)
Comment 3 Marc Véron 2017-05-05 10:54:52 UTC Comment hidden (obsolete)
Comment 4 Marc Véron 2017-05-07 07:11:47 UTC Comment hidden (obsolete)
Comment 5 Marc Véron 2017-05-07 07:49:07 UTC
Created attachment 63218 [details] [review]
Bug 18541 - Patron card creator: Add a grid to support layout design

Add a layout grid to patron card creator to figure out the positions of text
fields, barcode and images.

To test:
- Apply on top of patch 18465
- Go to Home > Tools > Patron card creator
- Edit or create a layout
- Turn on new choice 'Guide grid' in section 'General settings'
- Leave 'Units' unchanged
- Crate a PDF using 'Card batches'
- Notice that card is printed with a layout grid that reflects selected unit
  with each 5th and 10th line in different color, unit description displayed
  bottom left, card dimensions displayed bottom right in small print inside the
  layout grid
- Print PDF. Set printer settings in Adobe Reader or other PDF printing
  software to 'Actual size' to prevent scaling to printer's printable
  region
- Mesure out printed PDF and verify that grid corresponds to selecte unit.
- Go back to layout definition and choose an other unit, repeat steps
  to verify that grid respects selected unit.
- Go back to layout definition, turn grid off, create PDF, verify that grid
  does not display in PDF

Note for testers / QAers: Position of card elements (text, image...) do not
respect the unit, this will be fixed in Bug 18550
Comment 6 Alex Buckley 2017-05-08 23:48:22 UTC
Created attachment 63263 [details]
Unit description and card dimension on patron card batch

Hi Marc

This patch works well but I have one question:

You say in your test plan that the card batches should have the unit description displayed in the bottom left (which I have got) but the card dimensions should be in the bottom right however I have found mine is in the top right (see screenshot) is this what you intended?
Comment 7 Marc Véron 2017-05-08 23:53:55 UTC
(In reply to Alex Buckley from comment #6)
> Created attachment 63263 [details]
> Unit description and card dimension on patron card batch
> 
> Hi Marc
> 
> This patch works well but I have one question:
> 
> You say in your test plan that the card batches should have the unit
> description displayed in the bottom left (which I have got) but the card
> dimensions should be in the bottom right however I have found mine is in the
> top right (see screenshot) is this what you intended?

Hi Alex,
Yes indeed, I misaked with my description.
It should appear top right (as you describe) to indicate that the coordinate system is from bottom left = 0/0 to top right.
If you sign off, can you please change the test plan in the patch attached?
Thanks!
Marc
Comment 8 Alex Buckley 2017-05-09 10:04:32 UTC
Created attachment 63280 [details] [review]
Bug 18541 - Patron card creator: Add a grid to support layout design

Add a layout grid to patron card creator to figure out the positions of text
fields, barcode and images.

To test:
- Apply on top of patch 18465
- Go to Home > Tools > Patron card creator
- Edit or create a layout
- Turn on new choice 'Guide grid' in section 'General settings'
- Leave 'Units' unchanged
- Crate a PDF using 'Card batches'
- Notice that card is printed with a layout grid that reflects selected unit
  with each 5th and 10th line in different color, unit description displayed
  bottom left, card dimensions displayed top right in small print inside the
  layout grid
- Print PDF. Set printer settings in Adobe Reader or other PDF printing
  software to 'Actual size' to prevent scaling to printer's printable
  region
- Mesure out printed PDF and verify that grid corresponds to selecte unit.
- Go back to layout definition and choose an other unit, repeat steps
  to verify that grid respects selected unit.
- Go back to layout definition, turn grid off, create PDF, verify that grid
  does not display in PDF

Note for testers / QAers: Position of card elements (text, image...) do not
respect the unit, this will be fixed in Bug 18550

Followed test plan and it worked as intended
Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Comment 9 Marc Véron 2017-05-18 07:11:05 UTC Comment hidden (obsolete)
Comment 10 Marc Véron 2017-06-20 15:09:10 UTC
Still applies on current master.
Comment 11 Marcel de Rooy 2017-09-06 12:06:15 UTC
-        $patron_card->draw_guide_box($pdf) if $print_layout_xml->{'guide_box'};
-        $patron_card->draw_barcode($pdf) if $print_layout_xml->{'barcode'};
+        $patron_card->draw_guide_box($pdf) if $layout_xml->{'guide_box'};
+        $patron_card->draw_guide_grid($pdf) if $layout_xml->{'guide_grid'};
+        $patron_card->draw_barcode($pdf) if $layout_xml->{'barcode'};

Marc,
Please explain this change.
Formerly, as I understand, it flipped between layout_xml and layout_back_xml. Now you always set it to layout_xml. Do you change behavior for draw_guide_box and draw_barcode ?

Status change to reflect need for feedback only.
Comment 12 Marcel de Rooy 2017-09-06 13:08:22 UTC
Forgot to mention that qa tools reports a fail on pod somewhere too
Comment 13 Marc Véron 2017-09-06 13:29:44 UTC
(In reply to Marcel de Rooy from comment #11)
> -        $patron_card->draw_guide_box($pdf) if
> $print_layout_xml->{'guide_box'};
> -        $patron_card->draw_barcode($pdf) if $print_layout_xml->{'barcode'};
> +        $patron_card->draw_guide_box($pdf) if $layout_xml->{'guide_box'};
> +        $patron_card->draw_guide_grid($pdf) if $layout_xml->{'guide_grid'};
> +        $patron_card->draw_barcode($pdf) if $layout_xml->{'barcode'};
> 
> Marc,
> Please explain this change.
> Formerly, as I understand, it flipped between layout_xml and
> layout_back_xml. Now you always set it to layout_xml. Do you change behavior
> for draw_guide_box and draw_barcode ?
> 
> Status change to reflect need for feedback only.

Marcel,
Thanks for QAing and pointing this out.
Yes, it should be $print_layout_xml, not $layout_xml.

Follow-up patch will follow.

Marc
Comment 14 Marc Véron 2017-09-06 13:34:05 UTC
(In reply to Marcel de Rooy from comment #12)
> Forgot to mention that qa tools reports a fail on pod somewhere too

I can not reproduce with patch applied:


Processing files before patches
|========================>| 4 / 4 (100.00%)
Processing files after patches
|========================>| 4 / 4 (100.00%)

 OK	C4/Patroncards/Patroncard.pm
   OK	  critic
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  pod
   OK	  spelling
   OK	  valid

 OK	koha-tmpl/intranet-tmpl/prog/en/modules/patroncards/edit-layout.tt
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  spelling
   OK	  tt_valid
   OK	  valid_template

 OK	patroncards/create-pdf.pl
   OK	  critic
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  pod
   OK	  spelling
   OK	  valid

 OK	patroncards/edit-layout.pl
   OK	  critic
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  pod
   OK	  spelling
   OK	  valid
Comment 15 Marcel de Rooy 2017-09-06 13:41:36 UTC
(In reply to Marc Véron from comment #14)
> (In reply to Marcel de Rooy from comment #12)
> > Forgot to mention that qa tools reports a fail on pod somewhere too
> 
> I can not reproduce with patch applied:

Thx for checking. If you submit your follow-up, I will check again..
Comment 16 Marcel de Rooy 2017-09-06 13:43:14 UTC
Should be the pod for new sub draw_guide_grid btw..
Comment 17 Marcel de Rooy 2017-09-06 13:44:24 UTC
(In reply to Marcel de Rooy from comment #16)
> Should be the pod for new sub draw_guide_grid btw..

I understand that it will be hard to provide unit tests for this sub btw, as I should officially ask you for too. Maybe some advanced mocking session?
Comment 18 Marc Véron 2017-09-06 13:50:26 UTC
(In reply to Marcel de Rooy from comment #17)
> (In reply to Marcel de Rooy from comment #16)
> > Should be the pod for new sub draw_guide_grid btw..
> 
> I understand that it will be hard to provide unit tests for this sub btw, as
> I should officially ask you for too. Maybe some advanced mocking session?

Sorry, that is out of my scope. If you insist I will have to abandon this bug.
Comment 19 Marc Véron 2017-09-06 14:01:56 UTC
Created attachment 66873 [details] [review]
Bug 18541: (QA follow-up) for comments #11 and #13

This patch changes variable names as mentioned in comments #11 and #13
Comment 20 Marc Véron 2017-09-06 14:13:55 UTC
(In reply to Marcel de Rooy from comment #16)
> Should be the pod for new sub draw_guide_grid btw..

There aren't any PODs in Patroncards.pm, I think that is for historical reasons.

Does it make sense to add a pod just for one sub, or is it better to create a new bug "Add PODs to Patroncards.pm"?
Comment 21 Marcel de Rooy 2017-09-06 14:17:21 UTC
Created attachment 66876 [details] [review]
Bug 18541 - Patron card creator: Add a grid to support layout design

Add a layout grid to patron card creator to figure out the positions of text
fields, barcode and images.

To test:
- Apply on top of patch 18465
- Go to Home > Tools > Patron card creator
- Edit or create a layout
- Turn on new choice 'Guide grid' in section 'General settings'
- Leave 'Units' unchanged
- Crate a PDF using 'Card batches'
- Notice that card is printed with a layout grid that reflects selected unit
  with each 5th and 10th line in different color, unit description displayed
  bottom left, card dimensions displayed top right in small print inside the
  layout grid
- Print PDF. Set printer settings in Adobe Reader or other PDF printing
  software to 'Actual size' to prevent scaling to printer's printable
  region
- Mesure out printed PDF and verify that grid corresponds to selecte unit.
- Go back to layout definition and choose an other unit, repeat steps
  to verify that grid respects selected unit.
- Go back to layout definition, turn grid off, create PDF, verify that grid
  does not display in PDF

Note for testers / QAers: Position of card elements (text, image...) do not
respect the unit, this will be fixed in Bug 18550

Followed test plan and it worked as intended
Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 22 Marcel de Rooy 2017-09-06 14:17:25 UTC
Created attachment 66877 [details] [review]
Bug 18541: (QA follow-up) for comments #11 and #13

This patch changes variable names as mentioned in comments #11 and #13

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 23 Marcel de Rooy 2017-09-06 14:17:29 UTC
Created attachment 66878 [details] [review]
Bug 18541: [QA Follow-up] Add POD to Patroncard.pm

Just adding the POD framework to make qa tools happy. The authors
are encouraged to complete this information.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 24 Marcel de Rooy 2017-09-06 14:18:21 UTC
(In reply to Marc Véron from comment #20)
> (In reply to Marcel de Rooy from comment #16)
> > Should be the pod for new sub draw_guide_grid btw..
> 
> There aren't any PODs in Patroncards.pm, I think that is for historical
> reasons.
> 
> Does it make sense to add a pod just for one sub, or is it better to create
> a new bug "Add PODs to Patroncards.pm"?

I added a follow-up.
Please add another follow-up to complete the pod for the new sub.
Thanks.
Comment 25 Marcel de Rooy 2017-09-06 14:19:48 UTC
(In reply to Marc Véron from comment #18)
> Sorry, that is out of my scope. If you insist I will have to abandon this
> bug.

I will not insist. I recommend to push it, but the RM has final word.
Comment 26 Marcel de Rooy 2017-09-06 14:21:10 UTC
(In reply to Marc Véron from comment #14)
> I can not reproduce with patch applied:

You probably are not using the latest version of qa tools.
Comment 27 Marc Véron 2017-09-06 15:52:43 UTC
Created attachment 66887 [details] [review]
Bug 18541: (QA follow-up) for comment #24 (POD)

Add description to POD for draw_guide_grid
Comment 28 Marc Véron 2017-09-06 16:00:17 UTC
(In reply to Marcel de Rooy from comment #23)
> Created attachment 66878 [details] [review] [review]
> Bug 18541: [QA Follow-up] Add POD to Patroncard.pm
> 
> Just adding the POD framework to make qa tools happy. The authors
> are encouraged to complete this information.
> 
> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

I filed Bug 19264 for completing all PODs in Patroncard.pm
Comment 29 Marcel de Rooy 2017-09-07 06:50:26 UTC
Created attachment 66910 [details] [review]
Bug 18541: (QA follow-up) for comment #24 (POD)

Add description to POD for draw_guide_grid

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Amended: Moved the description from draw_guide_box to .._grid.
Comment 30 Jonathan Druart 2017-09-19 14:49:44 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 31 Fridolin Somers 2017-10-23 14:58:14 UTC
Enhancement not pushed to 17.05.x