GNOME Bugzilla – Bug 649284
Allow saving of Custom Reports without changing name, overwriting existing report
Last modified: 2018-06-29 22:57:35 UTC
I have added a built-in report to the list of Custom Reports by clicking "Add Report". Now I want to make a change to some of the options. I open the report from the Custom Reports menu and make the changes I want, but "Add Report" is grayed and I can't click it to submit my changes. The button only becomes available if I change the report name, but that creates a new report entirely when all I wanted to do is make a change to an existing report. Now I have to delete the old report, then edit the new report to the old report's name, save it again, then delete the duplicate with the "wrong" name.
See also http://gnucash.uservoice.com/suggestions/1539353
I am working on this feature.
Created attachment 248234 [details] [review] Patch introducing new function to be used as part of the solution This first patch introduces a function that will be used in the next patch. I have separated it to make it easier to read the patches.
Created attachment 248235 [details] [review] Patch implementing the new feature This patch actually implements the new feature. It goes a bit further than the request though: the new patch switches from one save button to two buttons: a Save button and a Save As button for reports. If you click the save button on an open report it depends if the report is already based on a custom report template or not: 1. the report is not based on a custom report template yet: the report is converted into a new custom report template and you are asked to enter a new name for the newly created custom report template. 2. the report was based on an existing custom report template: the existing custom report template is updated with the current options of the open report. If you click the save as button on an open report: the report is converted into a new custom report template and you are asked to enter a name for the newly created custom report template. As indicated on the mailing list, custom reports should have unique names for user convenience. They may be named after a standard report, but any given name can be used for one custom report template only. So if you change the name of a newly saved report into a name that is already in use, the name change will be refused.
Can I ask for a review before the weekend ? Saturday is the last day I'm available to make any improvements if necessary. And ideally if the implementation is accepted as is, it can still be part of the upcoming 2.5.3.
Do we really need both "Save" buttons in the toolbar, and can we have a shorter name for the one we keep, maybe just "Save Report"? Recognizing that the save filename is leftover from before, shouldn't we take this opportunity to remove the version number? When I try to save an already-saved report Gnucash hangs in scm_internal_hash_for_each_handle: 0 0x023ace26 in scm_ilookup ()
+ Trace 232189
The stack frames from scm_internal_hash_for_each_handle varies, but it didn't seem to ever return from there. That's on OSX. I'm checking F18, will report on that shortly.
On F18, no hanging on subsequent saves, but I got this crash when trying to save when ~/.gnucash/saved-reports-2.4 didn't exist. Touching it and trying again succeeded: Backtrace: In unknown file: ?: 0* [gnc:report-to-template-new #] In /opt/gnucash/share/gnucash/scm/report.scm: 667: 1* [gnc:report-to-template # #f] 629: 2 (let* (# # # #) (if # #)) ... 657: 3 (if (gnc:save-all-reports) (let # # templ-guid) #f) 657: 4* [gnc:save-all-reports] 689: 5 (let ((save-ok? #t) (temp-path #)) (gnc:debug "saving all reports...") ...) 695: 6* [rename-file "/home/john/.gnucash/saved-reports-2.4" ...] /opt/gnucash/share/gnucash/scm/report.scm:695:5: In procedure rename-file in expression (rename-file gnc:current-saved-reports temp-path): /opt/gnucash/share/gnucash/scm/report.scm:695:5: No such file or directory Failed to load key /apps/gnucash/general/account_separator: Configuration server couldn't be contacted: D-BUS error: Message did not receive a reply (timeout by message bus)
Thanks for the review. Here are a few responses already: 1. The crash on F18, when saved-reports-2.4 didn't exist: that was easy to reproduce. It was a slumbering bug in code I didn't touch. The code now got executed via a different code path. I have already committed a fix to trunk as it's not directly caused by my new code. 2. Do we really need both "Save" buttons in the tool bar ? I have considered this and decided to put both of them to make sure people notice the difference. Until now people have been used to the idea that you always effectively have to do a save as operation (ie go and change the name, then hit the save button). With only one button on the tool bar, this idiom sticks even if the button is now always enabled (and effectively is a save operation instead of a save as). IMO if only the save button was put on the tool bar it would be too easy for someone to mistakenly change an exiting saved report because she would expect it to save as a new report. I would consider removing the save as button again in the next major release, when the split between save and save as is more common knowledge again. 3. Can we have a shorter name ? Do you have icon and text enabled in the tool bar ? The name choice fits is a larger context, namely that the term "Custom Report" is hopelessly confusing. If you read the first section of our wiki page on custom reports, you will understand what I mean: http://wiki.gnucash.org/wiki/Custom_Reports I would like to reserve the name Custom Report in the future for reports that are not delivered with gnucash (the kind of report the wiki page is really about). So I was looking for another name for what happens when you click on the save button. I realize this has nothing to do with the request this bug report is about. So I'll shorten the name to "Save Report" for now and make an independent issue of the confusing names. 4. The last and most difficult issue is the stack trace you get on OS X. I currently don't have an OS X machine around to test this on. Do you have any additional information that could help me figure this out ? Something in the trace file perhaps ?
Created attachment 248479 [details] [review] Patch implementing the new feature, with shorter menu names This patch replaces the second patch. The only change so far are shorter menu names.
Some more testing on the hang: I sprinkled some (display "foo\n") statements around and determined that it's hanging in report.scm line 630: (let* ((custom-template-id (gnc:report-custom-template report)) More important, it happens only if there are two saved reports based on the same report. IOW: 1. Create a report 2. Click Save, accept the report name 3. Click Save As, accept the report name 4. Now click Save again, and Gnucash hangs. With *that* procedure, I'm now able to get it to hang in Fedora as well. If I kill Gnucash, restart it, and create a new report of the same type or open either of the saved reports and try to save it, that hangs too. It doesn't matter whether there are different options on the versions. I played with renaming: It seems that any renaming of the second report will prevent the hang: Putting a space before the '1', changing the '1' to a '2', adding a word in front, or renaming the first report to have '2' at the end (thus "Income Statement1" and "Income Statement2") all prevent the hang.
Yes, icons and titles. The shorter titles are a bit better, but they still take up a lot of space on the toolbar. I do understand wanting to make a visually-obvious difference from the old interface. One could just have the "save as" button, which would prevent users from accidentally over-writing a previous template, but then it's not obvious how to replace an existing one short of deleting it. I suspect some users won't look at the menu. OK, let's get some user feedback. I agree wholeheartedly with reserving "custom report" for reports that aren't part of the Gnucash distro, though I suspect that there are very few of those in the wild. It's way too hard to learn to write reports for any but the most determined to do. But that means that we need a new name on the menu and for the dialog box -- something that means "Saved Report Configurations" without being quite as stilted.
Created attachment 248528 [details] [review] Patch implementing the new feature, version 3 This is the third revision to implement the requested feature. In addition to the original implementation, it contains these fixes: - shorter names in the toolbar/menu - fix program hanging indefinitely when a template name already exists
(In reply to comment #10) > Some more testing on the hang: > > I sprinkled some (display "foo\n") statements around and determined that it's > hanging in report.scm line 630: > (let* ((custom-template-id (gnc:report-custom-template report)) > > More important, it happens only if there are two saved reports based on the > same report. IOW: > 1. Create a report > 2. Click Save, accept the report name > 3. Click Save As, accept the report name > 4. Now click Save again, and Gnucash hangs. > > With *that* procedure, I'm now able to get it to hang in Fedora as well. > > If I kill Gnucash, restart it, and create a new report of the same type or open > either of the saved reports and try to save it, that hangs too. It doesn't > matter whether there are different options on the versions. > > I played with renaming: It seems that any renaming of the second report will > prevent the hang: Putting a space before the '1', changing the '1' to a '2', > adding a word in front, or renaming the first report to have '2' at the end > (thus "Income Statement1" and "Income Statement2") all prevent the hang. Thank you very much for the additional details. With this information I was able to trace back the cause: it was a counter that got reset to 1 instead of incrementing in the function to generate unique names.
*** Bug 605032 has been marked as a duplicate of this bug. ***
*** Bug 672354 has been marked as a duplicate of this bug. ***
Created attachment 248529 [details] [review] Patch introducing new function to be used as part of the solution, version 2 This is a second version of the patch that introduces a new function. Compared to the original patch, this has changed: - slightly change the way a GValue variable is used. The original way worked fine on Fedora, but resulted in compilation errors on Windows. The new form does introduce a warning on linux, but this looks harmless and I considered that better than a failing build. I intend to look into the warning later (if noone beats me to it).
Great, that fixes it. My inclination is to commit these patches to trunk so that they can go into 2.5.3, which I'm planning to tag later today for release tomorrow. We can also close the bug and authorize the bounty, unless you'd like to keep it open while you chase down the warning.
Thanks for the feedback. I have committed the two patches to trunk myself now (to avoid issues with authorship). The bug can be closed. I will chase it anyway, but that will be after my holidays.
GnuCash bug tracking has moved to a new Bugzilla host. This bug has been copied to https://bugs.gnucash.org/show_bug.cgi?id=649284. Please update any external references or bookmarks.