Feature/printpreview

[mattiasmaahl]

This closes #258

WYSIWYG

What you see is what you get? 😄

The beginning

In the beginning there was a bug #258 , I set out to fix this issue and created a "one dialog to rule them all" kind of thing. I believe I'm there, almost, or rather this works and is WYSIWYG but I'm missing a few features that I would like to add to this.

• Styling of tables for one, I created a class for it but this needs some thinking-juice added to it on how to implement it. I'll get back to you on that in a later PR I believe.
• Secondly, Exporting to BB-CODE is no longer supported! This needs work if it is to remain, I don't really know what it is used for other than forum posts?! I'll implement it sooner rather than later as an option for export in the preview dialog though. Don't want to remove features here now would I!
• Adding support for graphics like RangeBars or piecharts for example, that would be awsome!

Going forward in to the near Future

In this code I have tried to implement a system to more easily move over to creating a "template" system where you can create your own templates and define your own layout on how and where you put things.\ Wouldn't that be cool! I have a few ideas on how to do it, but need to mix in more thoughts on that before I boil that to a PR.

EDIT: BtPage.markdown is deleted due to BtPage namespace was removed.

[matty0ung]

Am hoping to look at this in the next few days.

[mattiasmaahl]

Cool, I'll be available for comment, small fixes may be possible depending on all the other projects that I got going on at home during the next few weeks

[matty0ung]

I had a bit more of a look at this. I'd like to get input from @mikfire. On the one hand, from the screenshots you've shown, this is definitely a great improvement on current print preview. On the other hand, I have a couple of anxieties:

• A major one is that we don't yet have a solution for this to work in non-English languages (see // TODO: comparing ins->name() with these untranslated strings means this // doesn't work in other languages. Find a better way. )
• A minor one is it feels like we wrote a lot of code here to do the on-paper layout, and my instinct keeps telling me that this ought to be a solved problem where we can leverage some existing templating framework (eg mustache) to generate HTML that we then get Qt to print, ideally in such a way that differences in paper sizes (Letter, A4, etc) are handled for us. (This is by analogy with on-screen layout, where we use QML to do a lot of the work rather than do everything manually in C++.) OTOH, I haven't done any printing from C++, so I can't definitively say "It will be simpler if we do X", I'm just flagging up my nagging feeling!

[matty0ung]

I am having a deeper look at this to see if my two concerns can be addressed. (I know the first one is from existing code not the new patch.) Hope to have some constructive feedback this month.

[mattiasmaahl]

Sorry for not getting back to you earlier, I have been kind of offline the last 3 weeks! 😊

I have read your feedback and of course you can scrap the btpage class/namespace all togeather and use a textbrowser with HTML instead. Not a problem. My thinking was to have all printable pages as object that could maybe be stored as file-objects, but that motivation is also aplicable on HTML files, so any wah is fine by me.

I have vacation this week as well going back to work this coming monday. Cas address everything then. Your time and feedback is much appreciated.

[matty0ung]

No worries. Hope you had a good vacation!

I think we are already partly on the right lines, in that RecipeFormatter, InventoryFormatter and so on produce HTML. My instinct is to, as you say, use QWebEnginePage or similar to print this because then we hopefully get HTML, Print and PDF (with choice of page sizes for the latter) all for the same price (more or less). I've started playing around with merging a copy of your new code in Brewken, so I can better understand it, so I might have some more concrete suggestions soon.

[mattiasmaahl]

So I know you have been busy with the new database rework. But did you have any suggestions on how we want to proceed?

As I see it we can either keep the btpage namespace and have all the flexibility in the world the draw the printouts, or we can scrap that and go with HTML which would be easier in that it's a well known format and it does almost all I imagined the printouts to be. ( I say almost because I don't know if it can do all that I want). for example I had a thought that I would implement the "ranges" from the mainpage into the printouts as well at a later stage, but I don't know if HTML can do that, with btpage I know I can cobble it together, even if I have to draw everything my self. but this may not be what we, Brewtarget org, want in the end.

let me know and I'll act accordingly, I'm not married to any part of the code, I learned a bunch coding this anyway.

[matty0ung]

Sorry, I hadn't realised you were waiting on more input from me. Database work is pretty much done (just waiting on approval from @mikfire) but then I got side-tracked into BeerJSON.

At heart, my concern is that we generally want to avoid doing layout manually in C++ if we can because it means having to write so much boilerplate code. Where we can do stuff in QML or HTML (or similar), it's typically a lot easier to maintain and extend. (Sometimes for bits of dynamic layout we do end up writing C++ because of limitations in QML - eg to do the PostgreSQL / SQLite panel in the config settings pop-up. But it's typically a last resort.) It seemed to me that print layout would be potentially more complicated than on-screen layout because of page breaks (unless there's some magic in Qt that handles all that for us?), which felt like all the more reason to veer away from doing the layout in C++.

At the same time, I hear what you're saying about including nifty graphics etc. And if that's something a lot of people want, and if there isn't an easy way to include such things in an HTML/QML-generated layout, then, you're right, it would push us towards doing the more "manual" layout.

Would be good to get @mikfire's take on this.

[mattiasmaahl]

As I said, I don't know if it can be done, maybe it can, in HTML/QML. there may be a way to convert QT-objects to HTML somehow, need some more investigating.

But we can do this in multiple steps and implement the HTML for now, and see if there is a need for more "cool" graphics late down the line.

I'll wait to see if @mikfire wants to weigh in or not. Implementing HTML output instead of btpage is a simple thing. All the code is there just need to rearrange and delete a bunch.

[mattiasmaahl]

I have a Commit Ready that removes all btPage references and replaces the printout with HTML generated format. I have not made any changes to the HTML generation so that is what it is.

If I push that commit the printouts will not be pretty, but the will be WYSIWYG. That will open up for a future PR that fixes the HTML generation part of things.

[matty0ung]

That sounds good. So, if I've got the right end of the stick, we'd be doing a small(er) change initially, which fixes the UI but doesn't change the print output a lot, and leaving the door open to do something (maybe more complicated) later that changes what's actually printed.

[mattiasmaahl]

Yes, that is correct. I need to investigate if HTML can do what I want it to do regarding templates and graphics. Bit that's a later PR. Will push tomorrow.

[matty0ung]

Sorry for slow response - was back working on DB layer.

This looks good. A few very minor thoughts:

1. PlacingFlags / RelationalPlacingFlags / FixedPlacingFlags / etc could be removed from dev-doc/BtPage.markdown

2. Don't think we need the casts on (Unit const *)&Units::kilograms and (Unit const *)&Units::liters.

3. In the copyright comment in src/BrewDayFormatter.h, do we have the right dates and authors if it's a new file? :smile: Generally I try to put authors in alphabetical order by the way.

4. In the include guards, we don't want a leading underscore (replace _BREWDAYFORMATTER_H with BREWDAYFORMATTER_H etc) as I think names with leading underscores are reserved for compiler use etc.

5. I like how you've used HTMLgenerationFlags as a nice way of dealing with what would otherwise be too many different bools. :+1: Just on the name, for me, HtmlGenerationFlags would look better. Similarly FERMENTABLESFLAG would be better as FERMENTABLES_FLAG (or even just FERMENTABLES since we already get "flag" from the enum name). But I don't know if we standardised such things, so this is only a suggestion.

6. For bool operator&(HTMLgenerationFlags a, HTMLgenerationFlags b), I wouldn't bother making it inline and would put the definition in the cpp file rather than the header. The separation of implementation from interface is worth more than the miniscule performance benefit (which might even end up being 0 with a good compiler).

7. Similarly, instead of virtual ~PrintAndPreviewDialog() {} in the header, I would declare virtual ~PrintAndPreviewDialog() in the header and PrintAndPreviewDialog::~PrintAndPreviewDialog() = default in the cpp file. (You may think you will never ever ever modify such destructors, but one day you might when you're debugging some bizarre double-free crash. Ask me how I know. :smile_cat: )

8. Just a suggestion, but for the following block of code

   InventoryFormatter::HTMLgenerationFlags flags = ((checkBox_inventoryFermentables->isChecked()) ? InventoryFormatter::FERMENTABLESFLAG : InventoryFormatter::NOOPERATION) | ((checkBox_inventoryHops->isChecked()) ? InventoryFormatter::HOPSFLAG : InventoryFormatter::NOOPERATION) | ((checkBox_inventoryYeast->isChecked()) ? InventoryFormatter::YEASTFLAG : InventoryFormatter::NOOPERATION) | ((checkBox_inventoryMicellaneous->isChecked()) ? InventoryFormatter::MISCELLANEOUSFLAG : InventoryFormatter::NOOPERATION);

I think you could express it more concisely (and just as clearly) as something along the following lines:

InventoryFormatter::HTMLgenerationFlags flags {
   checkBox_inventoryFermentables->isChecked() * InventoryFormatter::FERMENTABLESFLAG +
   checkBox_inventoryHops->isChecked()         * InventoryFormatter::HOPSFLAG         +
   checkBox_inventoryYeast->isChecked()        * InventoryFormatter::YEASTFLAG        +
   checkBox_inventoryMicellaneous->isChecked() * InventoryFormatter::MISCELLANEOUSFLAG
}

See https://stackoverflow.com/questions/14034478/boolean-multiplication-in-c for why.

[mattiasmaahl]

Those are very good points, I'll look into them tomorrow, but that should be pretty straight forward. I'll get back to you tomorrow.

One note though. I removed the BTpage.markdown file entierly as it was not applickable anymore as all refs and files for that namespace/class has been removed.

[matty0ung]

https://github.com/Brewtarget/brewtarget/pull/608/files seems to think BTpage.markdown is still there, or is that some GitHub gremlin?

[mattiasmaahl]

Ok, I'll look into that. Was pretty sure I removed the file. Let med get back to you tomorrow.

[mattiasmaahl]

Just looked, I cannot see the file now. But will look closer tomorrow

[matty0ung]

It may be GitHub being overly clever and showing me an older version or something. I'll try clearing my cookies!

Télécharger Outlook pour Androidhttps://aka.ms/AAb9ysg

---

From: Mattias Måhl @.> Sent: Wednesday, September 29, 2021 9:25:21 PM To: Brewtarget/brewtarget @.> Cc: matty0ung @.>; Comment @.> Subject: Re: [Brewtarget/brewtarget] Feature/printpreview (#608)

Just looked, I cannot see the file now. But will look closer tomorrow

— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://github.com/Brewtarget/brewtarget/pull/608#issuecomment-930476356, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AESIX7EN6ID665W6WVAEXK3UENR2DANCNFSM472XAQXQ. Triage notifications on the go with GitHub Mobile for iOShttps://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Androidhttps://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

[mattiasmaahl]

Ok I have done some changes:

1. BtPage.markdown file has been removed entierly as it no longer applies, see commit: 1a103d85f89ce3fa4bd563ce51c5b49b657d8515
2. The Unit casts are from PR #587, but are now removed.
3. The Authors are now both in .h and .cpp and int Alphabetical order. Code in those files are written by the authors mentioned in the comment, I only moved their code around.
4. Changed the include guard _BREWDAYFORMATTER_H to BREWDAYFORMATTER_H
5. FLAG has now been removed from the enum names in InventoryFormatter::HtmlGenerationFlags
6. Implementation of OR and AND is now moved into the InventoryFormatter.cpp file.
7. ~PrintAndPreviewDialog() is now moved to PrintAndPrevewDialog.cpp and changed to be =default;
8. Flag generation has now been changed to use multiplication/addition instead of OR-ing IF:s. :)

[matty0ung]

Good stuff!