Fix date

[ederag]

Should fix #590.
This one has been unstaged for about three weeks
(my motherboard crashed, too old, had to change the whole computer, notably).

No warranty, that's untested.

Edit:
Rationale explained in #630 (comment) and #670 (comment)
They killed this PR for wrong reasons.

It's not perfect, but it's a much better starting point (as explained in my comments below).
Please do not try this PR itself (I gave up updating it at some point). The branch is better.
These instructions should give you a good working copy, as at least someone confirmed.

Last edit: In the end, they chose to merge #674 instead, just to show they are the boss, which is wrong in itself,
especially as #674 is not better.

[mwilck]

Can we get this in small, easy-to-review pieces, please? For example the fix for #590 separated out?

[ederag]

@mwilck Rebased on master. There was a conflict. If only PR #573 were merged.
Disclaimer: only a simple check has been done, please test thoroughly.

[mwilck]

I'm missing an explanation why this this fixes the problem. The renaming of the methods / fields doesn't help to fix anything, AFAICS. The last commit is the only one that might consitiute a fix, although I don't see why it would. My analysis after playing with this code is #590 (comment), and I don't understand why this PR would change anything about the double-application of the time delta I saw there.

[ederag]

Indeed, the first commits just clarified code, enough to find the simple bug fix (or so it seems ?).
Please test first, and report back.

[benjaoming]

Python 3.6.7 (default, Oct 22 2018, 11:32:17) 
[GCC 8.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from datetime import datetime
>>> datetime.now
<built-in method now of type object at 0xa346e0>
>>> datetime.now()
datetime.datetime(2020, 4, 27, 13, 16, 56, 954984)
>>> datetime.now().hday()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'datetime.datetime' object has no attribute 'hday'

[ederag]

Was this theoretical, or based on an actual test ?
Please look at the imports: from hamster.lib import datetime as dt

~/share/prog/python/hamster/src>GSETTINGS_SCHEMA_DIR=../build/data   python3 
Python 3.6.10 (default, Jan 16 2020, 09:12:04) [GCC] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from hamster.lib import datetime as dt
>>> now = dt.datetime.now()
>>> now.hday()
hday(2020, 4, 27)

[benjaoming]

ah okay, that's where the object came from! I didn't recognize hday, was just trying to be helpful, thinking this was part of the untested code :)

[matthijskooijman]

I also stepped in this at some point in the past, confusing python's datetime with hamster's datetime. Maybe some renaming could help here at some point.

[ederag]

Maybe some renaming could help here at some point.

And forget about compatibility with existing external code ?

[matthijskooijman]

I've also reviewed the code, and am also a bit puzzled. In particular:

• The first commits rename stuff, but without any rationale about why. I do not see a significant improvement in this rename, so without further motivation I am not inclined to merge that.
• I can't quite see what the last commits achieve exactly. One seems to improve handling of day vs hday, so times that are past midnight, but what does it fix exactly? The other implies it fixes Offset between calendar selection and cmdline #590, but I can't quite see how.

So, I am indeed also missing explanations and rationale for these commits. You call for testing and reporting, but in addition to having a fix that works, I also consider it important to understand how a fix works, which allows considering whether a fix is actually correct, proper and whether there might improvements possible. It also helps to do some more directed testing. Without understanding a fix, I would not be inclined to merge it (even if testing shows it works).

So, if you would like to see this merged, please provide some more explanations (ideally in the commit messages, so it will be available in the git history after merging) and rationale for making these changes.

In the meanwhile, I'll see if I can briefly PR to see if it actually fixes anything, but I'm also inclined to look at a more thorough refactor of the code to fix this (I'll submit a comment about that to #590 in a minute).

[matthijskooijman]

I also stepped in this at some point in the past, confusing python's datetime with hamster's datetime. Maybe some renaming could help here at some point.

[matthijskooijman]

I just checked out this PR and did a quick test, the last commit does seem to fix the problem for new facts, but not when editing existing facts (which does seem to make sense given the code changed). So, this is not actually a complete fix, and probably not the right fix either?

[ederag]

What are you doing exactly ?
When I edit an old fact from the april 21st, and change date (with the timeline arrows),
the start and end dates follow correctly here.

[matthijskooijman]

When I edit an old fact from the april 21st, and change date (with the timeline arrows), the start and end dates follow correctly here.

The problem is not the timeline arrows, those work for me just fine in master already. The problem is when editing the start date using the calendar underneath the "start" field. When I open up an existing item (I tried with one of today and yesterday), then open up the calendar and change the date, then the timeline (and I presume default date) get updated correctly, but the cmdline shows a start date which has the change applied twice.

This happens on existing and new items with master, and just with existing items with your PR applied.

[matthijskooijman]

This happens on existing and new items with master, and just with existing items with your PR applied.

Hm, I can no longer reproduce this: The bug now also occurs with new items with your PR applied. Not sure why that part seemed fixed before...

[ederag]

2nd part seems fixed in https://github.com/ederag/hamster/tree/fix-calendar-offset.
Cherry-pick from it, if you wish.
This PR branch is yours now, so you may force-push to ederag:fix-timeinput.
but please do not try to create any PR on my side. I'm out.
It's not a fork; the master branch is lagging behind to keep that clear.

[matthijskooijman]

#630 has a better fix for this issue, so I'm going ahead and close this, so we can focus on that fix instead.

[matthijskooijman]

Let me further clarify why I closed this PR:

• The PR as it is, does not fix the original problem completely, and as such is not ready to be merged.
• The PR contains half a dozen commits and it is not immediately clear to me from the commit messages what they do exactly and why. There is no overall documentation about what the rationale behind this fix is.
• @ederag has added a link to a branch with additional commits that supposedly do fix the problem, but again, it is unclear to me how the fix works exactly and I'm not so motivated to reverse engineer the fix (then I'd rather just spend that time to understand the code and come up with a fix myself).
• There is another PR in Adding activities in retrospect is very hard #630 that does fix the original problem and has IMHO easier to understand and review commits.

I realize that #630 is not yet ready, since it introduces some new problems, as pointed out by @ederag, but I still believe that fixing #630 is a better approach than digging into the commits in this PR and the linked branch, so I still think this PR should be closed and there is no real point in referring back to this PR. If @ederag (or anyone else, for that matter) wants to clean up their fix and submit it as a proper, well-documented, easy-to-review PR, then that's of course fine, but that will still leave this PR closed.

[ederag]

Perhaps not complete, but according to my tests, it does fix the main issue.

The correct stance would have been to either test it right away,
or to take it as written by me, the former maintainer, and merged before your arrival.

All the first commits made are clarifications of my intent, as already said in #597 (comment)
(that part code was work in progress,
for instance some was written before we introduced the clear separation between civil date and hamster day),
so there's no reverse engineering to do.
This makes this PR a much better start for any further improvement.

[matthijskooijman]

I've been reconsidering the code in this PR and the branch linked by @ederag. To keep discussion in the right place, I'm moving some comments from #630 that relate to this.

I wrote:

I also finally had a good look at @ederag's https://github.com/ederag/hamster/commits/fix-calendar-offset branch, and now I understand better what they are doing there. In fact, that branch already updates some uses of the hamsterday to use day (I was thinking about hday, but I guess day is already distinct enough from date, so that would be fine), so it seems we're in agreement about that part. I'm still having trying to puzzle out the reasons for some of those changes (lacking detailed commit messages), but I think the approach of renaming some things for clarity and then making some fixes make sense.

So, I'm going to try to make a new PR for this issue, based on @ederag's branch, adding more detailed commit messages, restructuring some of the commits and maybe doing a few things different (especially in the later commits).

Then @ederag wrote:

Good that you eventually reconsidered #597. You got it.

But I explicitly ask you not to tamper with my commits.
That's not OK. Their dates and styles are important. That's what future devs will see.
Cherry-pick or rebase as is,
then you may append your own commits, with your own style.

[matthijskooijman]

Their dates and styles are important. That's what future devs will see.

I agree that the commit separation, messages and message style is important, which is why I want to make some small changes to the separation and mostly make the commit messages more verbose, so those future devs will have an easier time understanding these changes. I'll also take care to preserve commit dates where appropriate.

If you insist that you do not want your commits to be modified in anyway (other than rebasing or cherry-picking), I do not consider them usable as-is and I would apparently need to create my own commits (some of which will end pretty much identical to yours). Where appropriate (i.e. for code copied from your commits) I'd credit you in the commit message, but not in the commit Author field then. If you prefer this, then I'll oblige, but it does not have my preference.

[ederag]

Boy. How could that be OK ?
Just take my commits, as is, like I asked.
As if I had committed those before leaving.
Responsibility is mine, if that appease you.

[matthijskooijman]

Just take my commits, as is, like I asked.

As I explained, I do not think the commit messages are verbose enough and some commits need minor restructuring, so this is not an acceptable option for me.

I'm going to assume this means you'd rather have new commits without you listed as the author, than modified commits with you listed as the author, so I'll do that, then.

[ederag]

you'd rather have new commits without you listed as the author, than modified commits with you listed as the author, so I'll do that, then.

No I explicitly deny you that right.

After the hundreds of hours I spent on this project, a minimum of respect would be expected.

[matthijskooijman]

After the hundreds of hours I spent on this project, a minimum of respect would be expected.

I'm trying my best to do this in a respectful and open manner, additionally discussing the approach I want to take and looking for options that would be acceptable for everyone. I get the impression that you consider your own commits and commit messages being fully perfect and that you consider that any change to them makes them worse, even without even knowing exactly what changes those would be. I'm sorry you feel disrespected, that is certainly not my intention.

No I explicitly deny you that right.

Ok, I've noted your intent and I'll see what I can do without copying any of your code. For the record, I do not think your denial is legally feasible (the GPL grants an irrevocable right), but I'll try to respect it regardless (but I expect some bits of code might end up identically anyway).

[ederag]

You are all so mistaken about how free software should work.

I'm trying my best to do this in a respectful and open manner

Respect is much more than civility.
And respect there's little in this whole thread.

Edit:
So he chose the "play the boss" path, with #674,
instead of showing the requested sign of respect.
Let them struggle 🙂.

---

I talk about ethics. They answer law. How could they miss the point by that much ?

I gave a lot to this project,
while being well aware of truths aptly summarized in The Paradoxical Commandments,
so I'd just be glad if this project succeeded, with a little more of my code or not.
This is not in question.
I know how GPL works (how could anyone smart underestimate me that much ?), and it's good for free software.

I just forbid to taint my commits with their names and verbiage,
and otherwise, (necessary precaution, as they are masters of half-truths)
denied anyone to claim that they followed my will,
which was explicit: merge my commits as they are,
as a sign of respect.

[rhertzog]

No I explicitly deny you that right.

You can't do that. The license explicitly permits anyone from reusing your code. I'm not sure why you are still here... if you don't want people to reuse your code and you only contribute poisonous remarks, then there's no reason for us to tolerate your presence here, whatever historical contribution you may have made.