Btw patch contains 5mm as page margin (while printing).

This is my personal choice.

In real world, 25mm (1 inch) margin are what people prefer.

So when you apply the patch, change 5mm to 25mm and apply.

Created attachment 273609 [details] [review]
Better Transaction reports - transaction.scm

Wow, those are many improvements in one patch. Thanks for your contribution.

However for better evaluation and to make it easier to understand what change was made for which reason I'd prefer to have this patch split up in a number of smaller patches, one for each improvement. Assuming you started from a git repository, you can use the git tools to do so. If you need help to get started with git's refactoring don't hesitate to ask on the gnucash-devel mailing list (to which you can subscribe via http://wiki.gnucash.org/wiki/Mailing_Lists )

Umm I can not create different git patches, but I will try explain each section of diff file here.

Diff has 74 lines added, 10 lines removed/replaced

Use this link as reference to diff (colored)
https://bugzilla.gnome.org/attachment.cgi?id=273609&action=diff&headers=0


Till line 127, the diff is just defines of new variables that are used

Line 127-138 is explained in point number 1 and 2 in first post above.

It is just a cosmetic change to table. No logic change. It also adds two extra "placeholder" rows with class=actheadstart (meaning account head start) and class=actheadend (account head end).

These extra rows are used in Javascript to detect different account sections. (will come below)


Line 355-361 is related to point 5 above and it applies better logic for calculating colspan that is used in generating "total" rows


Line 407-413- heading-list is now global variable so no more local, inside function (but its purpose remains same i.e. stores header row for table)


Line 443-451 - this is my opinion as per point 6 above. i.e. credit is actually debit and debit is actually credit.


Line 629-634 - adds new preferences for new features that I have added.

"Formats the table for printing each table on separate page" - each account/table is printed on different page.

"Use only one table for all accounts, instead separate tables for each Account" - this more or less switches table formatting to how it is currently. i.e. all accounts are embedded in single HTML TABLE.

"Additional Style to apply to document." - allows user to customize HTML stylesheet (if he is good in CSS)


Lines 1213-1221 since heading-list is now global variable, We set it here but we dont print it here. It will be printed for each account. (new line 146-149)


Lines 1418-1423 - self explanatory (see lines 629-634 above)


Lines 1509-1515 - adds <div> HTML element around table. To balance <div> elements which will be added by javascript hack.

New line 1566 puts contents of "Additional Style to apply to document." in that location.


Javascript code is basically a hack.. not an efficient solution but currently the best I could think of due to limitations with scheme HTML module.

Javascript here is what does most of the magic to add new features!

First it picks up existing <table> line which will be used for creating new <table> tags (new lines 1571 to 1573)

New line 1574 - then it replaces each row that has class "actheadstart" with a <div> and <table> start tag. Also adds <thead> start tag.

<thead> tag allows better printing, making headers appear on each page when transactions of account exceed more than 1 page.

Similarly, new line 1575, ends </thead>, </table> and </div> tags replacing each row that has class "actheadend"


So that is all!!

(You may now want to read points 1 to 9 in first page again to understand purpose of all changes!)

Amm, I did understand all the points in your original post. That was not why I requested you to split the changes.

I asked this from the point of code maintainability. For example, some of your changes should appear both as a bugfix in the next 2.6 release and equally in the future 2.8 release, while other changes can only go into the 2.8 release. Since you have mixed them all together in one patch I can't do that.

Also I wanted to discuss some of the improvements in more depth while others are no-brainers and can be committed immediately. But again since this is one big patch that's not possible.

But let me just give you some feedback as is then:
- I consider point 5 to be the fix for a bug. The code for that alone should be a separate patch so it can be applied to both 2.6.x and the development tree.

- Your point 6 may need evaluation by someone with more accounting knowledge.

- The javascript hack is clever. However I prefer not to use it. Introducing selfmodifying code makes the report very difficult to debug should that ever become necessary. Why didn't you do these html modifications in the scheme code ?

And lastly can you clarify why you can't create different git patches ? There are good improvements in your patch but I would like to see it presented in a form that's more manageable.

Ok, is it ok if I fork and send pull requests on github?

I can do that, I dont use (or have experience) with command line based git.

About Javascript hack, it is needed because current scheme table functions dont support adding <thead> tag. (atleast I failed to find anything on Google)

Without <thead> my purpose of creating this whole "patch" would not be achieved.

I dont know why you are scared of debugging for javascript.

The javascript I used is not a rocket science that will need lot of debugging, its mere search and replace code, it should not be difficult to debug in any case. (Although I doubt it needs anymore debugging)

By the way, if you want to see sample screen shots, I had posted them on mailing list here:
https://lists.gnucash.org/pipermail/gnucash-user/2014-April/054024.html

If javascript is not used, then whatever is written/shown in e-mail/screenshot, can not be done.

If you are not going to implement javascript code, then Point 2, 3, 4, 7 can not be done without <thead>, so point 9 (which reverses 2, 3, 4, 7) is also not required.

So if you plan not to use javascript then 1, 6, 8 are just minor 1-2 line changes / edits. Those you can implement directly instead of requiring patches.

only 5 has logic change. I can create patch for that if you want.

Ok I have created pull request on GitHub which has 4 patches as requested above by Geert.

Here is the link:
https://github.com/Gnucash/gnucash/pull/7

(In reply to comment #7)
> If you are not going to implement javascript code, then Point 2, 3, 4, 7 can
> not be done without <thead>, so point 9 (which reverses 2, 3, 4, 7) is also not
> required.
> 
> So if you plan not to use javascript then 1, 6, 8 are just minor 1-2 line
> changes / edits. Those you can implement directly instead of requiring patches.
> 
> only 5 has logic change. I can create patch for that if you want.

I am not against javascript per se, but using it to work around a missing tag in the existing html generator code is not ok.

Why:
1. it doesn't degrade gracefully in environments that don't have javascript available. Some users export their html code to be imported in other applications. With the added javascript code that modifies the report while it is being "run" in a webbrowser the reports are no longer consistent when imported in say LibreOffice Calc or other applications that more or less understand html, but have no clue of running the javascript.

2. Debugging the previous version of the report is straight forward: you have scheme code writing a static html page. The structure of this html page is what will be rendered. There is a one on one mapping between the two. You don't even need to render the report in an html capable browser. A text editor is sufficient. However this is no longer true with the javascript code. What you see in your text editor is no longer the report that will be presented to the user. You now *have* to open the report in some browser (embedded in gnucash or not) to be able to see the final result. Again I'm not against this in principle but I do not agree this is ok just because it can be done.

In this case there is a cleaner alternative: teach the html generating code to insert the <thead> tags you need. I realize this area is fairly complex. If you need help feel free to ask your questions on the gnucash-devel mailing list or on irc.

(In reply to comment #8)
> Ok I have created pull request on GitHub which has 4 patches as requested above
> by Geert.
> 
> Here is the link:
> https://github.com/Gnucash/gnucash/pull/7

Thank you for setting up a fork on github. I'll check it out though my comment 9 still stands.

Well in environment where Javascript is not understood, Javascript will not run anyway. So how will it modify the code?

If Javascript is not run then HTML remains same. Rendering is not affected.

I am just suggesting that to use features its better to open in browser. I am not putting any limitation that it has to be browser. Just that some feature will work and some not. But does not cause wrong rendering.

I had already checked html-table.scm if I could add thead.

But honestly (not trying to be rude), I am not going to go through complex scheme files just to add <thead> capability when I have a easier (javascript) solution available.

Just tried this myself:

Tried to open my report HTML in openoffice.

It does not run javascript but does not disturb any rendering.

Report just open fine.

Idea is:

if "viewer" is javascript capable then it uses powerful features of Javascript.

if "viewer" is not JS capable then it works just like its working now. i.e. without those features.

So overall nothing breaks.

(In reply to comment #11)
> Well in environment where Javascript is not understood, Javascript will not run
> anyway. So how will it modify the code?

It will not. That exactly my point. In gnucash you see a certain layout which is partly generated by the javascript code. When you import the same report into an environment that doesn't understand javascript you are not importing what you saw originally in gnucash, you are importing the report *before* javascript modified it.

> 
> If Javascript is not run then HTML remains same. Rendering is not affected.
> 
No, your javascript is inserting <thead> nodes. Your html does not remain the same.

> I am just suggesting that to use features its better to open in browser. I am
> not putting any limitation that it has to be browser. Just that some feature
> will work and some not. But does not cause wrong rendering.

I understand this. It is a known limitation of the embedded webkit engine (or our use of it at least) that it's not aware of page boundaries.

(In reply to comment #12)
> I had already checked html-table.scm if I could add thead.
> 
> But honestly (not trying to be rude), I am not going to go through complex
> scheme files just to add <thead> capability when I have a easier (javascript)
> solution available.

I appreciate you at least looked into it. And I agree our report code is really in for some serious refactoring and improvement.

It's complex, it doesn't separate structure from content, it's not using templates, it hardly uses css, javascript is currently only used for charts as it was cleaner to do it this way than what we had before (goffice).

But all that doesn't validate adding a blob of javascript just because that is easier for you. Again, I'm fine with javascript in principle and it looks like it's your native world.

However the way you are using it doesn't fit in the current project as a whole and as such I can't accept it.

Even though your patch is small, relatively simple and does what it is supposed to do it is a hack to work around the fact that the original report system doesn't provide the html tag you need. This adds a whole new layer of complexity where it should not. It opens the flood gates for even more of such small hacks eventually resulting in a terrible patchwork that is very hard to maintain as a whole. I will not allow that. It's hard enough already as it is.

Instead if you really want to work towards a more flexible and easier to layout transaction report I invite you look into eguile. Late in the 2.4 development cycle we started with an alternative report system based on eguile. It helps to alleviate most of the issues I mentioned earlier. Unfortunately only a couple of reports got ported to the new system. If you can convert the transaction report to eguile, you will gain a lot of flexibility to do what you are now trying to achieve with javascript: you have complete freedom over the html structure because it's stored in an separate template. All html tags are at your disposal. At the same time you can add css to your heart's desire to create a fancy layout. You can avoid pretty much all the old rendering functions in html-table.scm. The advantage is that structure, content and layout are managed separately which in the end results in code that is easier to maintain.

(In reply to comment #13)
> Just tried this myself:
> 
> Tried to open my report HTML in openoffice.
> 
> It does not run javascript but does not disturb any rendering.
> 
> Report just open fine.
> 
> Idea is:
> 
> if "viewer" is javascript capable then it uses powerful features of Javascript.
> 
> if "viewer" is not JS capable then it works just like its working now. i.e.
> without those features.
> 
> So overall nothing breaks.

I should have tested this myself, which I did by now. You are right that your code can still be opened in openoffice just fine. Which confirms your code is well written.

Nothing of this however changes my opinion with regards to the concept of the patch. It is not ok for me to use structural elements to fix layout issues. It is not ok for me that you need dynamic code to fix the static structure of the report.

Umm, I wish I could help but I can not. I know Gnucash is very helpful to me, but I have already done what I could. Learning eguile and changing everything is impossible for me.

Plus I am not accountant. I just created patches because I felt that's how it should be. i.e. thats how I wanted the reports to be. I may sound selfish but trust me this is not my line of work.

But still, how about this?

I add a checkbox as preference: "Use Javascript in reports (for modern browsers)"

If user wants better reports he can tick it, if old style reports he keeps in un-ticked.

Is it fine?

Created attachment 275199 [details]
Example report - printed from within Firefox

While I don't agree on the way javascript is used in the report I'm still interested in your improvements so I have played around a bit with the modified report.

I have attached a pdf file which is a printout of a report I have exported to html and then opened in Firefox. I used these settings:
- Insert page break before tables: yes
- Use single table for accounts: no

It looks like the printed report is splitting pages a bit too much: each account title is in a separate page.

Yet at the same time the account header is not repeated on each page so it's hard to determine for which account the current page is displaying information. The month headers do repeat correctly though.

Created attachment 275200 [details]
Transaction report in GnuCash - using separate tables

This pdf is from "Print to pdf" in gnucash. It preserves style information, something firefox does not. Webkit in gnucash however fails to detect page brakes as I mentioned before so ignore the fact that headers aren't repeated here.

This report is made with
Insert page break before tables: no
Use one table: no

To point out here is that the columns no longer align. The column titles which are inserted below the account name have different widths from the column titles under the month headings. This results in a rather chaotic layout.

I'm unsure why you added the headers under the account title anyway. Or am I using the report in ways that are unintended ?

> It looks like the printed report is splitting pages a bit too much: each
> account title is in a separate page.

> Yet at the same time the account header is not repeated on each page
> so it's hard to determine for which account the current page is displaying
> information.

Can you share the HTML instead?


Javascript splits on table. The report by Gnucash adds 1 extra table on top (i
dont know why that is required! It is done by gnucash somewhere and its not in
the scheme file) that is why first page looks like that.

Another problem we have is that the main account title is printed by some other
function and month wise is printed by some other function.

I could not understand which function prints title ("Credit notes" in your
example). Shifting line that adds "actheaderstart" row to that function will
solve the issue and account header will also repeat on every page.

(In reply to comment #19)
> This report is made with
> Insert page break before tables: no
> Use one table: no
> 
> To point out here is that the columns no longer align. The column titles which
> are inserted below the account name have different widths from the column
> titles under the month headings. This results in a rather chaotic layout.

This is because one-single table is split into multiple tables to use <thead> and pagebreaks. And then each table is "best fit" by browser.

Splitting table saves lots of pages when printing at expense of alignment. But then each account has its own headers so non-alignment does not cause

If you want equal alignment then you should check "Use on table" but then pagebreak you lose few features which depend on multiple tables.

You will realize that number of pages for printing increases if you tick "use one table."


> I'm unsure why you added the headers under the account title anyway. Or am I
> using the report in ways that are unintended ?

I have already mentioned this addition in point no. 2 of my first post on top. I thought that looks better.

Sorry I accidentally pressed commit without re-reading and "comment" has 2-3 unclear lines.

... count has its own headers so non-alignment does not cause

If you want equal alignment then you should check "Use one table" but then you lose few features which depend on multiple tables.

(In reply to comment #17)
> Umm, I wish I could help but I can not. I know Gnucash is very helpful to me,
> but I have already done what I could. Learning eguile and changing everything
> is impossible for me.
> 
Ok, I'm already thankful you took the effort to submit these patches. I really am. And I understand you are not in a position to dig int eguile and friends.

> Plus I am not accountant. I just created patches because I felt that's how it
> should be. i.e. thats how I wanted the reports to be. I may sound selfish but
> trust me this is not my line of work.
> 
Which is fine by me. Many general improvements started off as a personal itch. If the patches improve your work, please keep using them. But as I said in this case they are not compatible with the current structure of the gnucash reporting code.

> But still, how about this?
> 
> I add a checkbox as preference: "Use Javascript in reports (for modern
> browsers)"
> 
> If user wants better reports he can tick it, if old style reports he keeps in
> un-ticked.
> 
> Is it fine?

Unfortunately no. This is not a user issue. This is a developer issue. An extra preference would hide the improved report from the users but the maintenance burden is no less. On the contrary it's even more because the extra preference introduces yet another optional code path. I think users would generally be happy with the visual improvements your code bring to the table. It's me as a developer that is concerned about keeping the code base manageable for all the reasons I already tried to explain.

If reworking the report to work with eguile is not possible for you and you want to continue to use your personalized report I propose you make it a locally installed report instead of adding it to the main gnucash code base.

http://wiki.gnucash.org/wiki/Custom_Reports explains how you can set this up. It's basically a matter of giving the report a unique name and guid, moving it into your .gnucash directory and loading it via a config.user file. By loading it locally you avoid that future gnucash upgrades overwrite your own report.

Yes, I am already using it as Custom reports.

Thanks,

Created attachment 275209 [details]
Html for pdf "Example report - printed from within Firefox"

In reply to your comment 20. This is the html this is generated by gnucash.

Also note that the credit/debit story is slightly more involved than simply swapping them. The current order is correct for some types of accounts and incorrect for others.

More details on this can be found here: https://en.wikipedia.org/wiki/Debits_and_credits

A quick test on my system suggests that the existing order is correct for asset accounts, but wrong for income accounts. I haven't tested the other ones.

Thanks for HTML. My explanation was correct.

About credit-debit.. I am not accountant.. so I do not know all that exactly :)

I modified it for two reasons.

1) There was comment by original coder, even he was not sure if that was right order.
2) When I saw code, even I felt same.

Well in any case I have pushed your column calculating patch. Thank you very much.

Comment on attachment 273609 [details] [review]
Better Transaction reports - transaction.scm

See other bug comments for the motivation to reject this patch. I've also marked it as obsolete because the transaction report in gnucash 3.0 has been thoroughly overhauled so this patch won't apply any more.

And I'm now closing this report as obsolete for the same reason.

GnuCash bug tracking has moved to a new Bugzilla host. This bug has been copied to https://bugs.gnucash.org/show_bug.cgi?id=727617. Please update any external references or bookmarks.