GnuCash
Contact   Instructions
Bug 796812 - gnc_date_cell_get_date and gnc_date_cell_get_date_gdate have different date validation behaviour
Summary: gnc_date_cell_get_date and gnc_date_cell_get_date_gdate have different date v...
Status: RESOLVED FIXED
Alias: None
Product: GnuCash
Classification: Unclassified
Component: Register (show other bugs)
Version: unspecified
Hardware: PC Linux
: Normal normal
Target Milestone: ---
Assignee: ui
QA Contact: ui
URL:
Whiteboard:
Keywords:
: 796853 (view as bug list)
Depends on:
Blocks:
 
Reported: 2018-08-16 08:56 EDT by Geert Janssens
Modified: 2018-09-11 14:05 EDT (History)
5 users (show)

See Also:


Attachments

Description Geert Janssens 2018-08-16 08:56:24 EDT
Both functions will take the characters entered thus far in the cell and try to parse it into a valid date.

However The former will emit a warning if there is a parsing issue (like the parsed date being older than the read-only threshold of the book) while the latter doesn't. There is no documentation on why that is and I don't like such arbitrary inconsistencies.

However as both functions are used in different parts of the register code (which I still don't fully understand) I prefer to record it here for deeper investigation rather than just arbitrarily decide a consistent behaviour for both functions.
Comment 1 Geert Janssens 2018-08-16 08:57:09 EDT
Note this surfaced while reviewing Bob's PR to fix bug 796785:
https://github.com/Gnucash/gnucash/pull/397
Comment 2 Bob 2018-08-18 05:20:13 EDT
I thought I would comment here, when I looked at presenting a warning for dates out of range and also to fix a bug about silently changing invalid dates to the current year I thought I observed that the gdate form was only used for the help message and the other form actually changed the dates. This was slightly wrong as I had missed a call, see Bug 796813.

With this in mind I decided to add a parameter to gnc_parse_date that would enable presenting the warning message or continue to change the date silently. This was done to stop generating two warnings when dates were being changed.

So for the help text using gdate, there is no warning with the text silently being changed which I thought was not a problem. With the other form that does change the data, there is a warning which advises the date will be changed to this year or if read-only setting used to the last valid date for read only.
Comment 3 Geert Janssens 2018-08-18 09:17:24 EDT
Thanks for explaining this. I have missed this when you introduced the user warning.

However from an external user point of view we have two functions
gnc_date_cell_get_date
gnc_date_cell_get_date_gdate

We have several other examples in gnucash where we have two very similarly named functions indicating the same functionality but with a different variable format. In this case the name difference suggests the same functionality but with different return date format. But the functions are not doing the same thing: one emits a warning and resets The function names suggest a difference in returned date format.

That's not the case however: one will also emit a warning while the other won't. And this is poor design IMO. My first suggestion would be to make both functions consistent again by exposing the warn boolean at this level already (that is, make 'gboolean warn' a parameter to both gnc_date_cell_get_date and gnc_date_cell_get_date_gdate).

Taking a bit more distance I would even go further still and eliminate gnc_date_cell_get_date_gdate completely.
The motivation for this is a follows:
- we have gone through great lengths (well, John anyway) to come up with an interface to handle date/time consistently. This is found in gnc_date.h (for C). So I would use this as much as we can to ensure consistency in the whole application.
- GDate on the other hand is part of glib which we are trying to eliminate
- The gdate based function variant above is only used in 4 locations, two of them to print a date to be used in the help string and two times to save a date. For the last two you have already written the alternative in bug 796813, the former can equally be handled by a combination of gnc_date_cell_get_date and gnc_print_time64 instead of g_date_strftime.
Comment 4 John Ralls 2018-08-18 09:44:48 EDT
(In reply to Geert Janssens from comment #3)
> Taking a bit more distance I would even go further still and eliminate
> gnc_date_cell_get_date_gdate completely.

+1
Comment 5 Bob 2018-08-22 16:08:31 EDT
Fix hopefully in PR400 along the above lines.
Comment 6 Geert Janssens 2018-08-31 18:17:48 EDT
This has been merged and will appear in 3.3.
Comment 7 John Ralls 2018-09-11 14:05:32 EDT
*** Bug 796853 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.