On 29-06-13 16:52, John Ralls wrote:
On Jun 29, 2013, at 7:06 AM, Geert Janssens <[email protected]> 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
_______________________________________________
gnucash-devel mailing list
[email protected]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
