Created attachment 272903 [details] [review]
patch to be applied on top of GnuCash 2.6.2

Hi Geert,

here comes a solution proposal.

What is basically does
- assume a fixed width per label (30px showed good results to me)
- find out how many labels fit into the chart width without need to change anything
- if there are more labels in the list then is good for the display, start replacing each 2nd (or 3rd, or each 4th, a.s.o, depending on the number of ticks) by a space.

The biggest problem that I see is that the required div and mod operations change from guile 1.8 to guile 2.0.
I have implemented the 1.8 commands and placed the 2.0 commands next to it as a comment. As I am still working with 1.8 I did not manage to test the 2.0 commands.

Comment on attachment 272903 [details] [review]
patch to be applied on top of GnuCash 2.6.2

Hi Carsten. Thank you for your patch. This is an interesting approach. The quotient and remainder functions work on guile 2 as well by the way. There's no need to special case this for 1.8.

Unfortunately there is still an issue with your patch: when I resize an already open report (meaning change the display width in the report settings), there are suddenly no labels anymore.

Created attachment 273657 [details] [review]
Patch to be applied on GnuCash 2.6.3 or later

Hi Geert,

thanks for checking this out.

I removed the version differentiating stuff.

And when further looking into it, I see that the chart width options returns "inexact" integers, even if "exact" are give by the dialog.
So I put in a conversion inexact->exact to ensure that the quotient function always receives "exact" integers.
Now the labels should always show up.

But when playing around with it I noticed that this is solution is still not save from an "ugly" display:

a) Currently the chart width includes the "real" chart (i.e. the graph) width and the legend width. So, if the legend eats up a lot of width, the graph becomes quite slim and by that the use of the width calculation for the x-axis labels goes down the drain.

b) Even the bars start to overlap when the graph becomes too slim. My impression is that the bars do not go below a width of 10px.

Proposal:

aa) Change the placement setting for the legend from "outsidegraph" to "insidegraph". This makes the x-axis label calculation as introduced with this patch more reliable.
Note that the legend becomes transparent inside the graph and if the legend overlaps with bars then the mouse-over functionality (showing the tick value) still works.

aaa) The placement could be made adjustable to the user by introducing a display option for "inside(def)/outside".

bb) Introduce a check that the bars will not start to overlap, e.g. by assuming a bar can be reduced in width to min 5px but not any further. In case all bars with width 5px still overlap, then the width of the chart will be adjusted accordingly.

bbb) Alternatively, the view could switch from a bar chart to a line chart.

What do you think?

Created attachment 274204 [details] [review]
Patch to be applied on GnuCash 2.6.3 or later

there was a small typo in the html-linechart.scm part of the patch

Carsten, clearly the jqplot introduction is a source for many improvements in the graphical reports.

This report and your suggestions already touch many of them. I will add a couple of considerations here:

1. regarding the overlapping x-axis labels, which is the core of this bug. Jqplot itself comes with the capability of dynamically omitting x-axis labels when necessary but due to an unfortunate combination of circumstances this feature is not used. I do think though that we best work to use the native capability instead of implementing our own layer on top. The simple reason is that jqplot has got more data available to make a smart and clean decision on how many labels can be displayed. I'll come back to this part later.

2. Having the legend overlap with the chart can be offered as a user option however not by default and never an automatic choice. So IMO your option aaa is better than option aa. Given my first consideration it won't matter much for this bug report and can be implemented independently.

3. The minimum width of the bars is a limit I have introduced myself. It was not there before which sometimes resulted in bar charts for which the bars were only one pixel wide (and some bars even disappearing). I chose 10px because by experimentation. 10px gave a fair minimum bar width. If even less space is available the bars start getting packed closer together eventually overlapping. We could go for a 5px minimum as well but that would only move the problem area and I find the bars getting rather small with this minimum. That's obviously personal taste but I made the changes on request of some users that also prefer bigger fonts due to their eyes getting somewhat older.

4. Regarding the proposal to adjust the chart width when the bars tend to get too small. Personally I think it's not a problem that for large charts the bars start to overlap. The user can always manually adjust the chart width in the report options if he thinks the chart is too narrow. I have another idea in mind here though as well that will indirectly affect this whole bug: I think the fixed width options are very out of date and can be improved upon in many ways. For starters the chart should by default use the complete available width of the gnucash window. Purely for printing purposes we could keep width and height options but these should be in inches/centimeters instead of pixels. Additionally the chart could be made interactive. By default display on the full width of the gnucash windows, but have interactive (javascripted) buttons to zoom in/out. Obviously this as well is outside the scope of this bug report.

Now back the labels issue this all started with. I had been investigating this bug when I created it. It has been a couple of months and what I write below is from memory so there may be some inaccuracies.

jqplot has several renderers for the x-axis. The default renderer is is smart enough to dynamically calculate how many labels to print. This default renderer wasn't used because it takes numbers as labels whereas we are supplying strings (or more pedantically dates converted into strings). This just naturally evolved this way from the previous charting code.

So to use the default x-axis and its features we should figure out how we can pass numbers as x values instead of strings.

Strictly speaking each date can also be represented by a number, for example the number of seconds since epoch. GnuCash comes with plenty routines to handle this conversion. I think even that the report code explicitly does the inverse transformation.

So if we pass the dates as an integer number there is no need for a separate array of x-axis labels ('ticks'). jqplot will take the x values of all data points to compose the x-axis.

The real problem to solve is that we don't want to display the ticks as integers but rather as dates formatted in the user's chosen date format (locale or explicitly set in the general date preferences). Fortunately there is also a solution for this: jqplot allows to define a tick formatter function. This function takes the integer value of a tick and converts it to any other format. It's up to us to write a formatter function that can do the conversion from the date in integer format to the requested date string.

That's the principle. I haven't had the time nor energy to piece it all together.

Things to keep in mind:
- In this comment I'm always assuming that all bar charts have dates as x-axis. This may not be the case. But the principle remains the same. If the values on the x-axis can in some way be represented as integers we can write a javascript function to convert it in the proper string representation. If several formats exist we may have to pass the javascript functions as a parameter to the chart code instead of making it a fixed part of the chart code.

Created attachment 277674 [details] [review]
Patch to be applied on GnuCash 2.6.3 - 2

Hi Geert,

is this somewhat similar to what you have in mind?

It uses the jqplot.dateAxisRenderer.js for date display and the jqplot.cursor.js for zooming.

I find it quite comfortable on condition that your assumption that the x-axis always refers to date display is true.

Comment on attachment 277674 [details] [review]
Patch to be applied on GnuCash 2.6.3 - 2

Yes Carsten, this is very much what I had in mind and even better - the zoom feature is a nice bonus !

Regarding the assumption that all bar charts have date axes, this is currently valid. All current barchart reports use dates for the x axis. So this change is ok.

The final tidbit to improve is the date format used. It should match the date format chosen by the user in the date/time preferences.

This can probably be resolved by adding a formatString option to the tickOptions [1]. Which format to use will probably need to be determined in the scheme code and then passed as a parameter to the make-html-barchart function. What may be tricky is that the format strings in C/Guile may not map one-on-one on the format strings as listed in [1]. That needs careful consideration and you may need to write a conversion routine for this. I haven't checked this in detail myself yet so we may be lucky.

[1] The formatString options is explained here: http://www.jqplot.com/docs/files/plugins/jqplot-dateAxisRenderer-js.html

Comment on attachment 277674 [details] [review]
Patch to be applied on GnuCash 2.6.3 - 2

I have committed this part already. I consider this a big improvement anyway. Thanks a lot !

Created attachment 278798 [details] [review]
Patch to be applied on GnuCash 2.6.3 - 3

Hi Geert,

here my solution proposal for the date format setting.
I tried: It should apply nicely on the master branch :-)

!"§$%&#
Meaning: forget about my last comment - it does not at all apply nicely on master :-(

(in fact, git apply did not complain, which made me think that all is fine - but git status did not show any effect, so I re-created the patch, and now, eventually, it tells me that this is not possible at all...)

Created attachment 278814 [details] [review]
Patch to be applied on GnuCash 2.6.3 - 3 - rebased

this one should apply nicely on master

Comment on attachment 278814 [details] [review]
Patch to be applied on GnuCash 2.6.3 - 3 - rebased

Hi Carsten,

Thank you for the follow-up patch. I'm not sure what to do with it, so I asked the other devs to look into it as well.

Personally I'm not too happy with the cond on the date-format. I'm not sure it can be avoided, but perhaps it can. Do date format strings in javascript/jquery differ from date format strings in C ? If they are the same, we may just pass the C format string to the chart.

If they are different, you'll need to figure out a scheme to allow dates in the user's locale format as well.

Comment on attachment 278798 [details] [review]
Patch to be applied on GnuCash 2.6.3 - 3

Superseded.

Comment on attachment 278814 [details] [review]
Patch to be applied on GnuCash 2.6.3 - 3 - rebased

A one-liner convenience function? Seriously?

The function g_strdups the string. Where does that copy get freed?

Plus you can get the actual date format string you want with qof_date_format_get_string(). By duplicating that in Scheme you avoid the possibility of including the time in the string in the unlikely event that the date format is set to UTC, but you also force ISO format for the default case of the date format being set to the locale or not set at all; the C code uses the date format from the environment in that event.

Comment on attachment 277674 [details] [review]
Patch to be applied on GnuCash 2.6.3 - 2

This patch has been partly reverted due to issues with the dateAxisRenderer on bar charts.

Carsten, my apologies for pushing you into this direction. I really had high hopes of this combination.

(In reply to comment #15)
> (From update of attachment 277674 [details] [review])
> This patch has been partly reverted due to issues with the dateAxisRenderer on
> bar charts.
> 
Unfortunately that also means the labels will start overlapping again if the chart is too small for all the data... So we're back to square one.

No worries, such is live.

What is the most pitty thing about it: I prefer working with line charts, and with line charts this problem does simply not apply.
(one of the reasons why I did not see the reported problems by myself)

So my current thoughts go into the direction to build in line chart support into all chart reports and use the dateAxisRenderer again on line charts.

And the story could continue: Those who only work with stacked bar charts might get the option to select between the categoryAxisRenderer view and the dateAxisRenderer view.

Thinking twice:
Better add option "line chart" to all chart reports and an option "dateAxisRenderer" to all chart reports - then it is up to the user to decide about a suitable combination.

The remaining problems only affects those users that prefer unstacked bar charts an cannot fall back to any alternative presentation
 .....  "only" ...... ;-)

Created attachment 289317 [details] [review]
patch to be applied on GnuCash 2.6.4 - 1

Here come my third thoughts:

Let's assume we are starting with GnuCash 2.6.4 on top of which we have applied the patch
https://bugzilla.gnome.org/attachment.cgi?id=288288
from Bug 737815 "Graphs cannot be generated correctly".

First I propose to introduce a switch:
Unstacked bar charts will use the CategoryAxisRenderer,
stacked bar charts will use the DateAxisRenderer.

By this we get rid of the overlapping x-axis labels for stacked bar charts.

Created attachment 289318 [details] [review]
patch to be applied on GnuCash 2.6.4 - 2

Next I would like to propose to move the line charts to the DateAxisRenderer, too.

At the same time we could add support for line charts to other reports that are available as bar charts, as shown in this patch.

It renames the "Bar Chart" reports to only "Chart" reports that let the user decide between bar chart and line chart representation.

By moving to the line charts to the DateAxisRenderer we get should make sure that line charts do not have a problem with overlapping x-axis labels.

The above mentioned changes apply for Expense/Income/Asset/Liability Chart reports.

Created attachment 289319 [details] [review]
patch to be applied on GnuCash 2.6.4 - 3

Further we could add a on option to the Budget Bar Chart report to also allow for line chart representation, just for completeness.

Created attachment 289320 [details] [review]
patch to be applied on GnuCash 2.6.4 - 4

Not to forget about having the DateAxisRenderer based charts nicely harmonized with the preference date settings, we could use this little "creative" solution.

Created attachment 289321 [details] [review]
patch to be applied on GnuCash 2.6.4 - 5

Last but not least we could re-use the very first solution proposal, that we skipped in favour of the DateAxisRenderer, and apply it again for the unstacked bar chart representation.

Admitted, this will not suppress overlapping x-axis labels 100%, maybe 95%, but the final 5% percent should be "discovered" only by very special users.

This patch finalizes the series of my third thoughts.

In my eyes, we could reach a state which allows us to close this bug, or alternatively re-prioritize it to minor enhancement ... and wait.

If you agree on that I am happy to prepare a consolidated patch for the master branch.

Carsten, thanks for tne new patches. However they don't seem to apply cleanly to neither maint or master.

Perhaps this is because more recent commits. Can you check this ?

basically no problem, but:
How do we avoid that I am merging and lifting against a running target?

My original idea, in order to have a stable baseline, was to always develop against the latest release.
So the patches need to be applied after git checkout 2.6.4 (in this special case plus one more patch as described in comment 18).

Assuming that you did not do this checkout, the patches will fail most likely.
(If you did ... damn, then forget about the following sentences)

So the idea above seems to be sub-optimal.

What would be a better approach?

(In reply to comment #24)
> basically no problem, but:
> How do we avoid that I am merging and lifting against a running target?
> 
You can't. It's your responsibility as patch author to make sure it applies cleanly to the current head of your chosen branch. And continues to do so until it's accepted and merged.

> My original idea, in order to have a stable baseline, was to always develop
> against the latest release.
> 
That should be the current head of your chosen branch (be it maint or master depending on the type of patch you're creating). I'll explain below why that is.

> So the patches need to be applied after git checkout 2.6.4 (in this special
> case plus one more patch as described in comment 18).
> 
Assume I do apply them to 2.6.4. That will have to be on a new branch, because all existing candidate branch heads (maint, master) have moved on since that commit.

So at some point that new branch will have to get merged into maint (in this case since you were working from 2.6.4). During that merge the same merge conflicts as I encountered while attempting to apply your patches to maint head will pop up as well.

The problem there will then be that the merge is normally done by a maintainer (like me), not the patch author. As I haven't written the patch it's much harder for me to resolve the conflicts as it should be for you. So I would have to ask you there and then to do the conflict resolution.

> Assuming that you did not do this checkout, the patches will fail most likely.
> (If you did ... damn, then forget about the following sentences)
> 
As you can see I didn't :)

> So the idea above seems to be sub-optimal.
> 
> What would be a better approach?

The proper approach is that you work from the current branch head. That gives you the best chance to avoid merge conflicts. However even then there are situations where someone else will make a conflicting change before your patch is accepted. In that case the maintainer will ask you to rebase your work to the new branch head, letting you resolve the conflicts. This is unavoidable when multiple people work together on a common source base.

And yet it's still better to work from the (moving) branch head. Here's why: by always working from the latest branch head and regularly rebasing your local branches to the newest branch head, you will only get small conflicts at once to resolve, making the conflict resolution(s) relatively easy to manage.

On the other hand if you base your work on a fixed commit in time (the release commit), you risk that over time the conflict resolution gets more and more complicated because the amount of conflicting commits tends to grow over time and you have to deal with all of them at once in one merge action.

Created attachment 322963 [details] [review]
add function jqplot-x-axis-no-overlap

Hi there,

long time no patch :-)
Sorry, but real life was requiring all of my energy last year.

Browsing through the history, my short wrap-up summary is:
- we do not want the DateAxisRenderer anymore due to known issues
- we do not want the default axis renderer, because it takes numbers as labels whereas we are supplying strings (i.e. dates converted into strings)
- we do not want the default axis renderer, because it places the bars on the ticks and not between them

I have tried quite a bit to find a way to make the CategoryAxisRenderer dynamically reduce the number of ticks to avoid overlapping, but I could not find anything.
So my current conclusion is: This is at the moment not supported.

Which leads me to the conclusion, that we should fallback to the intial idea as described in Comment1, including the limitation connected to it (discussed in Comment2 to Comment6).
See patch Bug720934-Overlapping-x-Axis.patch for this, which is applying fine on the current master and maint branch.

All other ideas (like introducing line chart support to all graphical reports) should go into other bugzilla tickets.

Meanwhile I was pondering
bug330719 - piecharts drops negative data too - error message misleading 
a bit.

My thoughts were going in the direction of offering both, absolute and relative sizing to the user. I verified that width and height specifciations in % are working fine with jqplot.

But if the width/height is specified in %, the currently proposed solution for this bug720934 will fail :-(
Bottom line: DO NOT APPLY IT if there is a change to introduce relativ width/hight.

So we would need a width/height conversion from % to px, to make this solution work.

Any hints how to get there are highly welcome :-)

Correction for above comment:

I was pondering
Bug 331739 - Report graphs should not have to be scaled manually
instead.

The fact that the patch calculates the visible ticks at report generation and not dynamically at report display is one reason I was holding back on it, though I believe the basic idea is ok as a workaround.

As it happens I did an experiment yesterday to implement your calculation in javascript yesterday and the initial code seems to work.

The main advantage of this approach is that the javascript code has access to the internal html elements used by jqplot and hence can extract the exact width the chart will be (regardless of the position of the legend) and use that to calculate which ticks to render.

Additionally the code can be extended to recalculate the visible ticks dynamically should we at some point make the charts dynamically sizeable. My current code doesn't do that, but it should be fairly easy from my understanding.

The experiment still needs some cleanups before it is ready to commit.

Just tested with a report using width:100%; height:100%. The javascript based code continues to properly determine which ticks to render.

There is another bug that prevents us from using the 100% settings though: the chart is rendered very small. Opening the generated html file in Firefox does render the chart at full width/height.

It looks as if webkit in gnucash doesn't know what the full width/height of the window is. Perhaps that's a bug in our version of webkit or we are not configuring webkit properly. I don't know yet. In any case that's a separate bug.

> In any case that's a separate bug.

Bug 763640

Review of attachment 322963 [details] [review]:

The idea of pruning labels from the printable list is an interesting one. The implementation in guile has a couple of drawbacks as already discussed in the comments.

So I have just pushed an implementation written in javascript that essentially does the same thing without the limitations of the guile version because it's called at a much later stage in the report rendering, when the internal size of the chart is already known.

I have also slightly tweaked the algorithm used to determine whether or not a tick should be visible. The current implementation will skip ticks as soon as the distance between 2 ticks would become less than 25 pixels. In the original algorithm the minimum distance seemed to vary depending on the number of ticks and the size of the chart (I suspect due to an accumulation of rounding errors).

The code has been pushed to maint and should appear in the next stable release.

A few closing notes:
1. There's one theoretical case in which the x-axis labels won't be pruned properly: if no x-axis labels are specified at all. In that case jqplot will generate the x-axis labels internally and they won't be available to the current code. The only way to fix that would be to patch jqplot itself.

As said this is a theoretical case because all of our reports do set x-axis labels. I'm mostly adding it here for future reference in case gnucash would in the future have such a report. The work around is obvious as well: let gnucash set the x-axis labels at all times.


2. Untested is the behaviour of this code in case a horizontal bar chart is requested. I haven't found a report in gnucash that allows horizontal bar charts, but there may be one in the future.

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