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

Find a solution for application hang #52

Closed
smurfy opened this issue May 15, 2013 · 11 comments
Closed

Find a solution for application hang #52

smurfy opened this issue May 15, 2013 · 11 comments

Comments

@smurfy
Copy link
Owner

smurfy commented May 15, 2013

Some backends (mainly öbb and possible bahn.de binary) could cause a application hang.

Possible Reasons:

  • Parsing of the response (c++)
  • Converting response object to listmodel (qml)

Possible Fixes:

  • Move parsing code to thread (c++, maybe in parsermanager)
  • Create listmodels in c++ (also parsermanager)
@leppa
Copy link
Collaborator

leppa commented May 15, 2013

  • Create listmodels in c++ (also parsermanager)

I'd vote for this even if it's not the (main) cause of the bug. It's pretty easy to expose a list to QML nowadays by subclassing QAbstractListModel. This would also ease the performance optimization changes I mentioned in my comment to #20 by moving data out of QML files and making them more stateless.

@smurfy
Copy link
Owner Author

smurfy commented May 15, 2013

Yea i think both possible fixes should be combined, because building the list model should also run in a different thread for performance.

I think this is easy and fast to implement, i hope i find the time implementing this.

@smurfy
Copy link
Owner Author

smurfy commented May 16, 2013

Ok i looked into the threading stuff, it could be implemented but i'm still checking the best approach. Most samples (and the implementation for the calendar) based on single functions/tasks moved to threads not a whole class/object like the parser.

I found documentation about moving a whole object into a thread, but then the communication with that object gets complicated.

you can not longer call the parser directly, but need to either "invoke" the methods or use signals.
I will perform some tests.
Also this needs to be implemented in the backend manager, not the fahrplan class directly, because the fahrplan class gets instanced multiple times (per qml file)

@smurfy
Copy link
Owner Author

smurfy commented May 29, 2013

Ok, here is my implementation of the threading thing, what do you think?

After thinking about the listmodel thing i'm not sure if its really a good thing, where should we put it, its gui specific. There is the posibillity that one gui uses more or less or simple different data from the result object.

I tested with just the threading and the searching of connections looks well better on my N950.

(to test use Öbb.at, it uses a huge response xml)

@leppa
Copy link
Collaborator

leppa commented Jun 1, 2013

I quickly skimmed through the code - looks ok to me. Will try to take a closer look this weekend, if I have enough time.

After thinking about the listmodel thing i'm not sure if its really a good thing, where should we put it, its gui specific. There is the posibillity that one gui uses more or less or simple different data from the result object.

I would disagree on that. QAbstractListModel is very general and it doesn't depend on UI. Except for BlackBerry Cascades, it can be used directly on all supported platforms. But Fahrplan for BlackBerry doesn't use Cascades yet, so no problem here. And it's always possible to write platform specific adapter if necessary.

Regarding different sets of data - we don't have this case currently and if this different data comes down to only different sets of fields, then no problem at all - model can generally provide all possible fields and UI will use only the ones it needs.

Generally speaking, switching to directly using QAbstractListModel would simplify code and should improve the performance.

@smurfy
Copy link
Owner Author

smurfy commented Jun 1, 2013

Ok,
i will wait until you had time to check the changes out and test them on BB10 if possible.
If all works out ok i will merge the branch to master and we keep this ticket open for the QAbstractListModel part.

After some tests on Harmattan with just the thread changes, the application hang should no longer occure.

With general switching to QAbstractListModel do you mean get rid of all structs like "StationsResultItem" or what exactly do you mean :)

@leppa
Copy link
Collaborator

leppa commented Jun 1, 2013

i will wait until you had time to check the changes out

But don't wait too much: my spare time is quite limited nowadays, since becoming a father 12 days ago :-)

With general switching to QAbstractListModel do you mean get rid of all structs like "StationsResultItem" or what exactly do you mean :)

What I mean is converting, for example, StationsResultList into QAbstractListModel derived class. In this case all list fields that were properties of StationsResultItem before, will be exposed as roles of the model. Then this model can be directly "plugged" into QML ListView. onParserStationsResult handler will become obsolete. Additional benefit to this would be that all pages that display results will become stateless -> no need to create and keep them on application start -> lower memory footprint and faster startup time.

StationsResultItem might still be useful in case there is a need to retrieve all fields at once (e.g., returning the whole object contents the same as get() method of ListModel does).

@leppa
Copy link
Collaborator

leppa commented Jun 1, 2013

Generally, I would make the whole UI stateless. Currently there is some business logic present in UI and it needs to be copied and updated when changed for all platform UIs. Let's take departure station as an example.

It is stored as a string on UI side. Most probably, you noticed that it's not easy to change station from string into an object that can store more than just a name (e.g., ID): this change involves changing UI-side code or it will break. I was thinking how this can be fixed and one solution is:

  1. Instead of storing station name on UI side, expose it as a property of FahrplanBackend (let's call it departureStationName). This property is what the UI will display
  2. Add a method to FahrplanBackend, e.g. selectDepartureStation(index).
  3. Make station search results a QAbstractListModel and expose it as a property of FahrplanBackend too.
  4. When station search results are shown and user selects one of them, we know its index in the list - call selectDepartureStation() method with this index.
  5. FahrplanBackend will fetch station object by the provided index and store it internally as a departure station. It will also update departureStationName -> UI will update too.
  6. When search for journey is performed, FahrplanBackend will retrieve all necessary data from this stored object and use it to make a request.
  7. If we apply this approach to all station, time and other fields, searching for journey would be just calling searchJourney() (no parameters needed) and pushing JourneyResultsPage.

The benefit of this approach would be that if you need to add some additional data to station object it won't affect UI at all. UI will continue to display station name provided as a property by FahrplanBackend. UI will continue to select a station by its index in the list and FahrplanBackend will take care about what data it needs to retrieve/save from the list based on this index. Nothing will need to be updated on UI as long as you don't change display-related property names.

This is just a rough idea of what I see can be done. I'm sure other approaches exist as well.

@smurfy
Copy link
Owner Author

smurfy commented Jun 2, 2013

First of, congratz to your fatherhood.

About the stateless approach, i really like the idea, it will be quite a lot of work, but i think it would be really great.
My time is also limited, but i try to get started on it.

But i think for that i will create a separate ticket/issue

@leppa
Copy link
Collaborator

leppa commented Jun 2, 2013

Here's a quick conversion of FarplanFavoritesManager to QAbstarctListModel to illustrate what I mean :-)
Updated and checked UI for Harmattan and Symbian/BlackBerry. Also, "blindly" updated Ubuntu UI.

First of, congratz to your fatherhood.

Thank you.

But i think for that i will create a separate ticket/issue

Yes, it's quite some work so sounds like separate a issue is needed (and I can see you created one already).

@leppa
Copy link
Collaborator

leppa commented Jun 8, 2013

Ok, looked through 5a317f1 and it looks good. Also, quickly tested it on BB10 - UI stays responsive during search and parse stages. Didn't notice any regressions, so I think you can merge it.

@smurfy smurfy closed this as completed in 96ec22f Jun 9, 2013
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

2 participants