New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Not everyone wants to enter dates with the day first #480

Merged
merged 11 commits into from Mar 9, 2017

Conversation

Projects
None yet
2 participants
@lenlo
Contributor

lenlo commented Jan 29, 2017

My preferred date format follows the ISO standard (YYYY-MM-DD), but moneyGuru insists on me entering them with the day first, then month, then year, i.e. backwards from how they are written and how I normally would enter them. At first I thought this was a simple bug and fixed it to always be left-to-right in whatever format the user prefers, but then I came across an article on the moneyGuru website that promoted the day->month->year entry as a feature, so I added a preference that allowed the old behavior to be reenabled. I still let the default be left-to-right as this would arguably be the most "natural" and expected behavior.

lenlo added some commits Jan 11, 2017

Fixed date widget to always start in the leftmost field
The date widget would always start in the day field when entering dates.
That's very backward for those of us that use the standard ISO-8601 date
format beginning with the year: 'yyyy-MM-dd'. Fixed it to start at the
leftmost field instead, regardless of the format.
Added a preference for the user's preferred date entry order
* Added a entry_format parameter to the creation of DateWidgets that will
  tell them in which order the dates should be entered.
* Added a setEntryFormat class method to DateWidget that will override the
  individual instances' settings.
* Added a preference called DayFirstDateEntry that will set the DateWidget's
  entry format to d->m->y if set to true. Otherwise, it will follow the
  user's date format's natural left-to-right order.

@hsoft hsoft self-requested a review Jan 29, 2017

@hsoft

This comment has been minimized.

Show comment
Hide comment
@hsoft

hsoft Jan 29, 2017

Owner

I don't have enough time to review this one right now, but it looks interesting! Will probably be useful to many. My preliminary comments are:

  • I'm lukewarm about having this preference being stored as a class variable. I know that because of the way that the DateWidget is instantiated, it might be tough to do otherwise, but still, lukewarm :)
  • It seems over-engineered to me to hold a whole date format for date entry. A simple bool would do I think because nobody's ever going to want anything else than the display format as their entry format (except for the original d -> m -> y of course :) )!
  • Unit tests.
Owner

hsoft commented Jan 29, 2017

I don't have enough time to review this one right now, but it looks interesting! Will probably be useful to many. My preliminary comments are:

  • I'm lukewarm about having this preference being stored as a class variable. I know that because of the way that the DateWidget is instantiated, it might be tough to do otherwise, but still, lukewarm :)
  • It seems over-engineered to me to hold a whole date format for date entry. A simple bool would do I think because nobody's ever going to want anything else than the display format as their entry format (except for the original d -> m -> y of course :) )!
  • Unit tests.
@lenlo

This comment has been minimized.

Show comment
Hide comment
@lenlo

lenlo Jan 31, 2017

Contributor

I'm lukewarm about the whole idea to do date entry in any other way than left-to-right, but sure... ;-)

Storing the override in the class seems like the obvious place to me, but if you have a better suggestion I'm open for it. I assume you don't want the class to reach out to the app and grab it from there. That would introduce an unnecessary dependency and make the code less compartmentalized.

I agree that the entry_format parameter to the DateWidget's constructor might be overkill and I could remove that if you want (or feel free to do so yourself). But that won't actually make much of a difference to the rest of the code. As long as DateWidgets take a format parameter, they also need to remember which order to fill in the day/month/year fields on an individual basis.

The unit tests. OK. Let me look into that...

Contributor

lenlo commented Jan 31, 2017

I'm lukewarm about the whole idea to do date entry in any other way than left-to-right, but sure... ;-)

Storing the override in the class seems like the obvious place to me, but if you have a better suggestion I'm open for it. I assume you don't want the class to reach out to the app and grab it from there. That would introduce an unnecessary dependency and make the code less compartmentalized.

I agree that the entry_format parameter to the DateWidget's constructor might be overkill and I could remove that if you want (or feel free to do so yourself). But that won't actually make much of a difference to the rest of the code. As long as DateWidgets take a format parameter, they also need to remember which order to fill in the day/month/year fields on an individual basis.

The unit tests. OK. Let me look into that...

@lenlo

This comment has been minimized.

Show comment
Hide comment
@lenlo

lenlo Feb 3, 2017

Contributor

Updated the DateWidget tests to cover both date entry orders d->m->y and left-to-right (for yyyy-mm-dd).

Contributor

lenlo commented Feb 3, 2017

Updated the DateWidget tests to cover both date entry orders d->m->y and left-to-right (for yyyy-mm-dd).

@hsoft

This comment has been minimized.

Show comment
Hide comment
@hsoft

hsoft Feb 4, 2017

Owner

I'm not sure we're talking about the same thing with regards to my 2nd point. What I meant is not to remove the constructor, but rather to make it a bool rather than a format. It would make the mechanism much simpler. Everything would stay pretty much the same except that if the flag is True, a _next() call would become the equivalent of calling right().

Also, it seems that tests you've added are false positive. left() and right() already work independently if the entry format. Those tests are redundant with others because they weren't failing before you added your changes. A more meaningful test would be:

    def test_type_2(self):
        self.w.type('2')
        eq_(self.w.text, '2   -06-12')

(and it would help to add the same test in TestCaseYYYYMMDDWithDot because it seems that we lacked that coverage)

Owner

hsoft commented Feb 4, 2017

I'm not sure we're talking about the same thing with regards to my 2nd point. What I meant is not to remove the constructor, but rather to make it a bool rather than a format. It would make the mechanism much simpler. Everything would stay pretty much the same except that if the flag is True, a _next() call would become the equivalent of calling right().

Also, it seems that tests you've added are false positive. left() and right() already work independently if the entry format. Those tests are redundant with others because they weren't failing before you added your changes. A more meaningful test would be:

    def test_type_2(self):
        self.w.type('2')
        eq_(self.w.text, '2   -06-12')

(and it would help to add the same test in TestCaseYYYYMMDDWithDot because it seems that we lacked that coverage)

@lenlo

This comment has been minimized.

Show comment
Hide comment
@lenlo

lenlo Feb 5, 2017

Contributor

Yeah, you're right, left_left() and test_right() are arguably redundant (although test_selection() is still useful).

I've now replaced left and right with a type test as per your suggestions (albeit with a few more characters) and added a similar one to TestCaseYYYYMMDDWithDot.

I'm sorry, but I'm less convinced about the value of replacing the entry_format parameter with a bool. You can't connect it to the preference as there may already be a plethora of DateWidget objects that already have been instantiated on the screen (and won't change) when/if the preference is changed. This means there won't be any change of behavior for them until the sheet is closed and reopened. However, you could dump the entry_format constructor parameter and change the setEntryFormat class method to (say) setDMYEntry which takes a bool instead of a format. I'm a bit hesitant as the code for the more general case already has been written and I would argue that it's not that complicated. But I could take a look at it if you feel strongly about it...

Contributor

lenlo commented Feb 5, 2017

Yeah, you're right, left_left() and test_right() are arguably redundant (although test_selection() is still useful).

I've now replaced left and right with a type test as per your suggestions (albeit with a few more characters) and added a similar one to TestCaseYYYYMMDDWithDot.

I'm sorry, but I'm less convinced about the value of replacing the entry_format parameter with a bool. You can't connect it to the preference as there may already be a plethora of DateWidget objects that already have been instantiated on the screen (and won't change) when/if the preference is changed. This means there won't be any change of behavior for them until the sheet is closed and reopened. However, you could dump the entry_format constructor parameter and change the setEntryFormat class method to (say) setDMYEntry which takes a bool instead of a format. I'm a bit hesitant as the code for the more general case already has been written and I would argue that it's not that complicated. But I could take a look at it if you feel strongly about it...

@hsoft

This comment has been minimized.

Show comment
Hide comment
@hsoft

hsoft Feb 5, 2017

Owner

although test_selection() is still useful

I agree

you could dump the entry_format constructor parameter and change the setEntryFormat class method to (say) setDMYEntry which takes a bool instead of a format

This is precisely what I meant in my previous comment.

I'm a bit hesitant as the code for the more general case already has been written and I would argue that it's not that complicated

Code is written once and read many times. Re-writing to simplify is very often worth it. Individually, it's true that the difference in complexity is not that great, but this complexity adds up when someone new to the codebase tries to wrap her head around it.

Thanks again for this feature. I haven't found the time yet to do a proper review (try it for real), but I will do soon.

Owner

hsoft commented Feb 5, 2017

although test_selection() is still useful

I agree

you could dump the entry_format constructor parameter and change the setEntryFormat class method to (say) setDMYEntry which takes a bool instead of a format

This is precisely what I meant in my previous comment.

I'm a bit hesitant as the code for the more general case already has been written and I would argue that it's not that complicated

Code is written once and read many times. Re-writing to simplify is very often worth it. Individually, it's true that the difference in complexity is not that great, but this complexity adds up when someone new to the codebase tries to wrap her head around it.

Thanks again for this feature. I haven't found the time yet to do a proper review (try it for real), but I will do soon.

@lenlo

This comment has been minimized.

Show comment
Hide comment
@lenlo

lenlo Feb 5, 2017

Contributor

Simplified the code as per your request.

Contributor

lenlo commented Feb 5, 2017

Simplified the code as per your request.

@hsoft

This comment has been minimized.

Show comment
Hide comment
@hsoft

hsoft Feb 22, 2017

Owner

I've tested it functionally on Qt and it generally works well. The only small problem I see is that the new option is False by default. It should be True to match previous behavior.

I'll be testing the Cocoa part soon and we'll be good to merge.

(sorry for the review delay, I've been busy)

Owner

hsoft commented Feb 22, 2017

I've tested it functionally on Qt and it generally works well. The only small problem I see is that the new option is False by default. It should be True to match previous behavior.

I'll be testing the Cocoa part soon and we'll be good to merge.

(sorry for the review delay, I've been busy)

@hsoft

This comment has been minimized.

Show comment
Hide comment
@hsoft

hsoft Feb 22, 2017

Owner

Works fine on OS X too.

Owner

hsoft commented Feb 22, 2017

Works fine on OS X too.

@lenlo

This comment has been minimized.

Show comment
Hide comment
@lenlo

lenlo Mar 3, 2017

Contributor

You're right, it should be True to match the previous behavior. However, is that really the right choice? I can understand the argument that those who have gotten used to the present behavior would rather not have it change underneath them, but what about all future moneyGuru users who likely will find an unexpected entry order confusing and annoying (like I did)? To me, the natural choice would be to have left-to-right entry as the default and let those who want a different entry order change it themselves. After all, that's how you enter dates in all other applications (and on paper, etc).

Contributor

lenlo commented Mar 3, 2017

You're right, it should be True to match the previous behavior. However, is that really the right choice? I can understand the argument that those who have gotten used to the present behavior would rather not have it change underneath them, but what about all future moneyGuru users who likely will find an unexpected entry order confusing and annoying (like I did)? To me, the natural choice would be to have left-to-right entry as the default and let those who want a different entry order change it themselves. After all, that's how you enter dates in all other applications (and on paper, etc).

@hsoft

This comment has been minimized.

Show comment
Hide comment
@hsoft

hsoft Mar 3, 2017

Owner

Default behavior is much more efficient, entry-wise. Most of the time, when you're entering many transactions, the only part of the date that changes is the day, so you're saving yourself 4 whole keystrokes. That ends up adding to a lot.

Owner

hsoft commented Mar 3, 2017

Default behavior is much more efficient, entry-wise. Most of the time, when you're entering many transactions, the only part of the date that changes is the day, so you're saving yourself 4 whole keystrokes. That ends up adding to a lot.

@lenlo

This comment has been minimized.

Show comment
Hide comment
@lenlo

lenlo Mar 3, 2017

Contributor

True, day-first entry can save you a couple of keystrokes, but only one or two as far as I can tell. To enter a different day of this month, you'd only need to press the right arrow once to get to the day subfield if your preferred date format is MM/DD/YYYY and twice if it's YYYY-MM-DD. That might be a good reason to have day-first entry available as an option, but as I see it, the more natural left-to-right order still ought to be the default -- just like automatic placement of a decimal mark is an option too, but the default is to enter it manually.

BTW, shouldn't it always be possible to enter the date separator character to move between the date components? It looks like you only can do that if you have manually entered a date component value today. E.g. with YYYY-MM-DD, just entering '-' doesn't do anything unless I first enter a year value, then '-' moves to the month. It seems to me that it should move to the month right away using the default value for the year. I don't think I changed that, but would you like me to roll that into this fix too?

Contributor

lenlo commented Mar 3, 2017

True, day-first entry can save you a couple of keystrokes, but only one or two as far as I can tell. To enter a different day of this month, you'd only need to press the right arrow once to get to the day subfield if your preferred date format is MM/DD/YYYY and twice if it's YYYY-MM-DD. That might be a good reason to have day-first entry available as an option, but as I see it, the more natural left-to-right order still ought to be the default -- just like automatic placement of a decimal mark is an option too, but the default is to enter it manually.

BTW, shouldn't it always be possible to enter the date separator character to move between the date components? It looks like you only can do that if you have manually entered a date component value today. E.g. with YYYY-MM-DD, just entering '-' doesn't do anything unless I first enter a year value, then '-' moves to the month. It seems to me that it should move to the month right away using the default value for the year. I don't think I changed that, but would you like me to roll that into this fix too?

@hsoft

This comment has been minimized.

Show comment
Hide comment
@hsoft

hsoft Mar 4, 2017

Owner

What about going gradually? We start by introducing the new date entry mode, and then, in a subsequent release, make it the default if it's a popular demand. What do you think about that?

As for your suggestion, I think it's a good one. I can't think of a possible downside to it. So yes, if you want to work on this, I'll happily merge.

Owner

hsoft commented Mar 4, 2017

What about going gradually? We start by introducing the new date entry mode, and then, in a subsequent release, make it the default if it's a popular demand. What do you think about that?

As for your suggestion, I think it's a good one. I can't think of a possible downside to it. So yes, if you want to work on this, I'll happily merge.

@lenlo

This comment has been minimized.

Show comment
Hide comment
@lenlo

lenlo Mar 7, 2017

Contributor

To be honest, you own this project so you can do what you like with it. But no, I don't think that's a good idea. I like simplicity and I like consistency between applications. I like to see dates formatted and entered the same way regardless of where I do it. It's fine to have features that break this as options, but they should be just that: options. The default should (IMHO) be the "normal" and expected way of operating. Only if the win in breaking a pattern clearly is significant would I think it worth pushing it onto the user from the start. In this case, it seems to save the occasional keystroke for (some) users, but not (again, IMHO) enough to warrant it being the default due to the consternation and confusion that comes with it.

But the choice is yours.

I'll look into making the other change we talked about.

Contributor

lenlo commented Mar 7, 2017

To be honest, you own this project so you can do what you like with it. But no, I don't think that's a good idea. I like simplicity and I like consistency between applications. I like to see dates formatted and entered the same way regardless of where I do it. It's fine to have features that break this as options, but they should be just that: options. The default should (IMHO) be the "normal" and expected way of operating. Only if the win in breaking a pattern clearly is significant would I think it worth pushing it onto the user from the start. In this case, it seems to save the occasional keystroke for (some) users, but not (again, IMHO) enough to warrant it being the default due to the consternation and confusion that comes with it.

But the choice is yours.

I'll look into making the other change we talked about.

Made pressing the date separator character always move to the next su…
…bfield

...even if nothing else has been typed yet (= keep the prefilled text, i.e.
today's date).
@lenlo

This comment has been minimized.

Show comment
Hide comment
@lenlo

lenlo Mar 8, 2017

Contributor

I was about to say that I just removed the check for _flush_buffer() success before proceeding to go to the _next() subfield when entering the date format separator. It did the job and seemed harmless enough, but then I noticed the failed test. Could you tell me what this is about? Any reason why I shouldn't just replace "0 " with "12"?

    def test_type_sep(self):
        # There was a crash when a sep-caused _flush_buffer would be called with an invalid value
        self.w.type('/')
        eq_(self.w.text, '0 /06/2008') # don't do anything (stay on DAY)
Contributor

lenlo commented Mar 8, 2017

I was about to say that I just removed the check for _flush_buffer() success before proceeding to go to the _next() subfield when entering the date format separator. It did the job and seemed harmless enough, but then I noticed the failed test. Could you tell me what this is about? Any reason why I shouldn't just replace "0 " with "12"?

    def test_type_sep(self):
        # There was a crash when a sep-caused _flush_buffer would be called with an invalid value
        self.w.type('/')
        eq_(self.w.text, '0 /06/2008') # don't do anything (stay on DAY)
@hsoft

This comment has been minimized.

Show comment
Hide comment
@hsoft

hsoft Mar 8, 2017

Owner

It looks like the test comment is not descriptive enough. Before your change, the behavior was, once a partial date is entered, to only accept input that would complete that partial date. Back then, I felt that allowing a "revert to old value on invalid input" was too confusing behavior-wise.

But now that we allow separator character to be used to skip to the next field without entering a new value, the correct behavior of that test would indeed be to revert to its previous value and change its focus to the next field.

Owner

hsoft commented Mar 8, 2017

It looks like the test comment is not descriptive enough. Before your change, the behavior was, once a partial date is entered, to only accept input that would complete that partial date. Back then, I felt that allowing a "revert to old value on invalid input" was too confusing behavior-wise.

But now that we allow separator character to be used to skip to the next field without entering a new value, the correct behavior of that test would indeed be to revert to its previous value and change its focus to the next field.

Updated the date_widget_tests to account for the new behavior when en…
…tering

the date separator to an "empty" buffer (i.e. allow it and move to the next
subfield).
@lenlo

This comment has been minimized.

Show comment
Hide comment
@lenlo

lenlo Mar 8, 2017

Contributor

OK, thanks. I've updated the test accordingly.

Contributor

lenlo commented Mar 8, 2017

OK, thanks. I've updated the test accordingly.

@hsoft

hsoft approved these changes Mar 9, 2017

Looks good, works well, thanks for working through my reviews. I will, however, change the default value for that new preference.

@hsoft hsoft merged commit 403cf6a into hsoft:master Mar 9, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

hsoft added a commit that referenced this pull request Mar 9, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment