Skip to content
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

[Ubuntu] [bahn.de] Shows trains which do not depart on the current day of the week #185

Open
nikwen opened this issue Apr 8, 2015 · 14 comments

Comments

@nikwen
Copy link
Contributor

nikwen commented Apr 8, 2015

Lately, I was on a journey from Nuremberg (Germany) to Munich on a Sunday and the app displayed a train which, according to the app, was supposed to depart at 12:10 h. However, I found out the hard way that it only departs on Saturdays.

Result: Had to wait one hour in Nuremberg for the next train.

@leppa
Copy link
Collaborator

leppa commented Apr 8, 2015

If you go to bahn.de website and search for the same connection there, does it return train at 12:10?

We show the information as provided by the service. So if bahn.de returns that train in the list of journeys, there's nothing we can do about that.

Unless Fahrplan, somehow, sent wrong date / time to bahn.de.

@nikwen
Copy link
Contributor Author

nikwen commented Apr 8, 2015

@leppa Thanks you very much for your reply.

The bahn.de website doesn't display the train. I'll try to find another case so that you can reproduce the bug.

@leppa
Copy link
Collaborator

leppa commented Apr 8, 2015

I can see there's a train from Nuremberg to Munich at 13:10 on Sundays. That's exactly one hour difference.
Did your issue happen this Sunday (05.04) or the Sunday before (29.03)?
I have a feeling that it might be somehow related to daylight saving time change.

@nikwen
Copy link
Contributor Author

nikwen commented Apr 8, 2015

@leppa I had exactly the same thing in mind as it happened on Sunday, 29th March.

I'll send you a screenshot I took back then if you want one.

@nikwen
Copy link
Contributor Author

nikwen commented Apr 8, 2015

Here is proof that it is in fact related to the daylight saving time change.

This is the screenshot I took on the day before the journey (some personal information removed):

fahrplan

This is a screenshot I took right now from the bahn.de website:

bahn de website

Comparing the train numbers clearly shows that the time was displayed incorrectly in the application.

I'm not sure how it looked the day of the journey itself as I mainly used the screenshots I took to save bandwidth.

@leppa
Copy link
Collaborator

leppa commented Apr 8, 2015

Thanks for the screenshot. Maybe, changing the phone date will reproduce the issue. Some of us will take a look at this issue.

I can also see that train comments are cut on the right. @mzanetti, can you enable word wrapping on the label?

@nikwen
Copy link
Contributor Author

nikwen commented Apr 8, 2015

Thanks. :)

Give me a moment and you'll have a PR with word wrapping. ;)

@nikwen
Copy link
Contributor Author

nikwen commented Apr 8, 2015

Another thought: If changing the date back doesn't help, one can always look at trains after the next daylight saving time change.

@nikwen
Copy link
Contributor Author

nikwen commented Apr 8, 2015

The pull request for word wrapping: #186 :)

@benzea
Copy link

benzea commented Apr 26, 2015

Hey, had the same issue, and I would say the reason might be the toTime function in the parser (which means that changing the date on the phone only changes some aspects).

nikwen, the first train you took, was it a 4:XX long train ride? Looking at the code I would expect that any duration that lasts for more than 2 hours will be displayed an hour too long if the phones date is on the change to summer time. For all other connection the trains date is used, so it will always be wrong.

All that means that hafas actually returns the hours and minutes on the day with daylight saving time already accounted for. It would probably be enough to modify the toTime functions of the hafas parser to use QDateTime::setTime() with the QTime of the calculated hours/minutes. i.e. the change below:

    return tmpDateTime.addSecs(((hours * 60) + minutes) * 60);

becomes

    return tmpDateTime.setTime(QTime(hours, minutes);

A small improvement might be to use a QTime in the first place to store the duration instead of relying on a QDateTime object there.

@benzea
Copy link

benzea commented Apr 26, 2015

Reproducing the issue is trivial. Just do a search on 25th of October, and all trains will be displayed an hour early (well, after the first 2:59 o'clock).

EDIT: It looks like Fahrplan uses the phones current timezone for all calculations. So the effect is visible when your local timezone changes daylight saving and it not when Europe/Berlin changes. That is likely only an issue if you try to store the times for other uses (e.g. by inserting them into the calendar).

@nikwen
Copy link
Contributor Author

nikwen commented Apr 26, 2015

@benzea No, the 5:XX hours which were displayed were correct. The journey would have lasted from 10:XX to 15:XX (opposed to 09:XX to 14:XX which the app displayed).

I took the screenshot on the day before the journey.

@benzea
Copy link

benzea commented Apr 26, 2015

On So, 2015-04-26 at 05:15 -0700, Niklas Wenzel wrote:

@benzea No, the 5:XX hours which were displayed were correct. The
journey would have lasted from 10:XX to 15:XX (opposed to 09:XX to
14:XX which the app displayed).

I took the screenshot on the day before the journey.

Makes sense then. Any search on the day will display wrong durations.

Two more things:

  • Please use QTime for the duration.
  • The QDateTime should be switched to use UTC or better set the timezone
    based on the backend that is used. It really shouldn't use the devices
    timezone, as the device might be in a different timezone and then bad
    things will keep happening.

Edited: Right, QTime canot handle date changes … well, I created a patch that should fix the issue. Is not properly tested though.

benzea added a commit to benzea/fahrplan that referenced this issue Apr 26, 2015
This commit switches the internal clock of the hafas provider to use
UTC. hafas already provides times that are adjusted for daylight saving
so no further adjustment needs to be done by the parser.
The patch also uses 1970-01-01 as the reference day so that a day switch
during reesult generation won't result in wrong duration displays.

This commit fixes issue smurfy#185.
@smurfy
Copy link
Owner

smurfy commented Apr 27, 2015

@benzea thanks for looking into this, feel free to create a PR once finished and tested.
Main thing to test is if it still works if you have a journey > 24h

benzea added a commit to benzea/fahrplan that referenced this issue May 3, 2015
This commit switches the internal clock of the hafas provider to use
UTC. hafas already provides times that are adjusted for daylight saving
so no further adjustment needs to be done by the parser.
The patch also uses 1970-01-01 as the reference day so that a day switch
during reesult generation won't result in wrong duration displays.

This commit fixes issue smurfy#185.
benzea added a commit to benzea/fahrplan that referenced this issue May 3, 2015
This commit switches the internal clock of the hafas provider to use
UTC. hafas already provides times that are adjusted for daylight saving
so no further adjustment needs to be done by the parser.
The patch also uses 1970-01-01 as the reference day so that a day switch
during reesult generation won't result in wrong duration displays.

This commit fixes issue smurfy#185.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants