Report system legacy

[Geert Janssens]

On 29-06-13 16:52, John Ralls wrote:
> On Jun 29, 2013, at 7:06 AM, Geert Janssens <janssens-geert at telenet.be> wrote:
>
>> Thanks all for your feedback. I'll keep from this that duplicate names among custom reports are not desired. To which I agree.
>>
>> I originally asked this question in the scope of one of the bugs in the GnuCash bounty program:
>> Allow saving of Custom Reports without changing name, overwriting existing report.
>>
>> Before I go into more detail, let me start with this: the current code is not able to prevent duplicate custom report names either. Don't believe me ? Try this:
>> - open a new report
>> - edit it's name in the report options and hit ok
>> - edit it's name again, resetting it to the original name and hit ok
>> - save the report
>> You can do the same thing by creating a new report and changing its name to the name of any existing custom report. There is no validation on the name.
>>
>> So the unique name requirement is a new requirement, which I see independently of the bounty requirement of being able to resave changes to an existing report template. I hope we can agree on this. I do intend to look into this as well, but not right now.
>>
>> I have been massaging the reports code for about a week now to come up with a satisfactory solution. You can not imagine what a can of worms that code is...
>>
>> To make the rest of the discussion a little bit manageable, allow me to first make a distinction between reports and report templates: each menu option you see in the reports menu is a report template. The moment you open one such menu you instantiate one instance of such a template. This is a report. "Custom reports" are also report templates, which have a parent template and a set of custom options. When you open a "custom report" you create an instance of one template, hence you have a report again. When you "save" a report, what you really do is creating a template based on the parent template and the current set of options for this report. In this discussion, we're constantly on this edge between report templates and reports.
>>
>> While looking at the custom reports code I found several issues with it, not only the inconvenience of having to change the name all the time before resaving. I came to the conclusion that simply adding a dialog box asking if a report should be overwritten is only shifting the problem to the next annoyance.
>>
>> Since I would be working on this code anyway, I wanted to eliminate several of these annoyances at once.
>>
>> So I worked from the file system metaphor that was already referred to in this discussion. Most programs have two Save options: simply Save and then Save As. You use the first to save your changes to an existing file, you use the second when you want to save your changes in another file. This is much more in line with how humans think than the current custom reports logic where you first have to change a name and only then can save it.
>>
>> So that's my general idea so far: make the custom reports logic more like a file manager. So far I have created two independent save buttons for reports as well: a save and a save as button which behave as you would expect: save will update the custom report template the current report is based on. Save as will prompt the user for a name and create a new template with this name. If the report to be saved is not based on any custom report template, the save button will behave as a save as button, just like a file manager save button would.
>>
>> The name prompt dialog is an improved version of the custom reports dialog, which also now allows to rename any existing report. Using this dialog makes it easy to see which custom report template names do exist already, so it becomes easier to generate a unique name. The old system relied on you to know which name is unique or not.
>>
>> Note that this solution implies that you know which template a report is instantiated from. The current code doesn't keep track of this. The obvious thing to do is to add a parameter to the report record for this. And the most obvious parameter to store is the custom report template's guid.
>>
>> So far a unique name is not enforced yet in my new code. But since you see the names of all existing reports when you save a new one, it's easy to maintain this manually.
>>
>> Now regarding this unique name enforcement, I'd like to think out lout a bit here.
>> With the new implementation, we risk duplicate names both when a user hits "Save As..." or when she uses the custom reports dialog to rename a report. So I'll just work with the generic situation of renaming a report.
>>
>> Suppose I have two report templates called "TemplateA" and "TemplateB" and there are reports currently instantiated for both templates. Next I try to rename TemplateB to TemplateA. What should I do ?
>>
>> a. delete TemplateA and rename TemplateB to TemplateA, probably after user confirmation ? That means that old TemplateA's guid is lost, and any open report based on it is no longer based on any custom report template.
>>
>> You could consider updating the open reports while renaming a template, but you can't. Templates are shared across all books, so at best you can update the reports in the currently open book. The issue could still pop up in another book.
>> An even stronger reason no to attempt updating open reports: what if TemplateA and TemplateB are not based on the same parent report ? So you now have one template "TemplateA", and some reports that claim to be instantiated from it, although they come from totally different templates originally.
>> Each time you hit save on either report, TemplateA will effectively swap parent template in addition to options. So depending on which report you last saved, your custom template will instantiate a totally different report. Perhaps this is an uncommon situation, but it may cause lots of confusion for a user that accidentally gets into it. So I don't think it's a good idea.
>>
>> b. delete TemplateA, rename TemplateB to TemplateA and set TemplateB's guid to old TemplateA's guid. In this variant, suddenly all open reports that were based on TemplateB won't be based on any template anymore. This is probably a very bad idea.
>>
>> c. If the new name already exists, simply refuse to continue. Ask the user to change the name again. This may be the simplest to implement, but in reality this will result in situations exactly like case a, except it's more cumbersome to the user: if you refuse to overwrite a template, the user can first delete that template and then rename the new template to the old one. All issues you get in case a will repeat here.
>>
>> In summary, I don't think we can avoid some loss of report to template links. So which option looks the most user friendly here ? I would think option a.
> Is there really a Report class? Isn't a Report an HTML page that results from running the ReportTemplate's query, any summarizing functions, and its display logic? Reports aren't saved, right? When one loads a Book with open reports, isn't the
> ReportTemplate retrieved and the Report HTML regenerated?
Hi John,

Thanks for your feedback.

Just for clarity, the definitions of report-template and report are 
purely in guile code (src/reports/report-system/report.scm). Our c code 
is totally unaware of this. It just passes around opaque SCM objects. I 
don't even know where the html page appears in this picture. It's not in 
this part of the code yet. A "report" in report.scm is defined as a 
record containing a report-type (which is the guid of a base report 
template), a report-id (just a number), a structure holding the report 
options, a reference to the editor widget and some housekeeping flags.
>
> Where I'm going with this is that if a report is if a Report is open it will just sit there until the user tries to reload it (which would include loading a Book with an open report). If the ReportTemplate is changed, the Report gets re-created based on the new ReportTemplate, right? If the ReportTemplate is gone, which is currently possible if one deletes a "custom report" in the dialog box, I would hope that Gnucash displays a nice "Can't find the Report" in the HTML page rather than crashing.
This doesn't happen, because the custom report's template id is never 
stored in a report record. Instead the base template on which the custom 
report template is based on is stored in the report record. So when you 
reopen a report (by reopening a book), the report currently doesn't even 
know it ever was instantiated from a custom report. It will 
reinstantiate from the base template. I believe this was originally done 
exactly to prevent unexpected crashes when the custom template got deleted.

This results in a two-level template hierarchy. The base templates are 
all the report definitions that ship with gnucash, or that a user loads 
manually (what I would consider a true "custom report"). What is called 
custom reports now are the reports a user can save from within the 
program. Which really aren't much more than a reference to a base report 
and a custom set of default options. The code keeps this hierarchy 
shallow. There will never be a custom template based on another custom 
template. Which I think is good exactly to avoid the template is deleted 
issue.

But I needed to keep track of which custom template a report is 
instantiated from, so I have now added an extra parameter in the report 
record to keep track of which custom report was optionally instantiated 
when the report was generated. I'm still keeping this parameter 
separately from the "report-type" which will always hold the core 
template to prevent the crashes you describe.

If the custom report gets deleted, the reference to it in the report 
record becomes invalid. But this is still no problem, because the rest 
of the code will treat an invalid reference as no reference at all. Or 
put another way: if you delete a custom template, any report that was 
instantiated from it will now be treated as if it was instantiated from 
the custom template's parent (or base) template and everything continues 
to work.
>
> So for your rename problem, don't allow renaming TemplateB to TemplateA: Require the user to explicitly delete TemplateA. Don't change the GUID for TemplateB, and retrieve report specs by GUID rather than name. The behavior for open Reports based on TemplateA is then the same as it is now (unless it crashes Gnucash, which should be fixed first).
>
> Regards,
> John Ralls
>
>
>
Given how things currently work I think it's even ok to allow renaming 
TemplateB to TemplateA, added a proper warning, explanation and user 
confirmation. It will have the exact same effect as  having the user 
delete TemplateA and then rename TemplateB to TemplateA in two separate 
steps. Other than that I agree that guid should be used as key. That's 
already the case now.

Geert