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

Clicking on a Container takes a long time to open details #974

Closed
Ads20000 opened this issue Aug 13, 2017 · 22 comments · Fixed by #990
Closed

Clicking on a Container takes a long time to open details #974

Ads20000 opened this issue Aug 13, 2017 · 22 comments · Fixed by #990

Comments

@Ads20000
Copy link
Contributor

Ads20000 commented Aug 13, 2017

There's significant lag when clicking on a Container before the details on that Container are opened. This is especially obvious after you start up Phoenicis.

@plata
Copy link
Collaborator

plata commented Aug 14, 2017

Phoenicis is collecting the information about the container from the file system. Therefore, this is highly depending on your disk drive etc. I currently cannot think of a better solution for this. If you have any ideas, please let us know.

@madoar
Copy link
Collaborator

madoar commented Aug 14, 2017

I'm not sure if the containers are the problem. I believe, that one of the problems here is the "update engine" combobox.
To create this combobox POL 5 needs to load all engines, which is quite some work.

@Ads20000
Copy link
Contributor Author

Ads20000 commented Aug 14, 2017

I'm using an SSD (got this laptop recently, the previous one was HDD, the difference is very noticeable!) so disk read-write speed is pretty good!

Well the problem wasn't present in POL 4 (not even with HDD)...

@plata
Copy link
Collaborator

plata commented Aug 14, 2017

@madoar that's true. But how do we solve it?

@madoar
Copy link
Collaborator

madoar commented Aug 14, 2017

I think we should add an EngineManager class, which is created only once.
This class is responsible for the management of all engines, including the fetching of the available engines online, managing which engines are installed and removing them.
This would allow us to do the fetching process only once.
What do you think?

@plata
Copy link
Collaborator

plata commented Aug 14, 2017

How can this EngineManager know if there is a new engine available?

@madoar
Copy link
Collaborator

madoar commented Aug 14, 2017

It should check at startup time, ideally in a background thread.

About the "how" itself:
I'm not sure how POL 5 currently detects what wine versions (i.e. our current engines) are available, but I think this is connected to #973. Maybe we could use the approach I've suggested there?

@plata
Copy link
Collaborator

plata commented Aug 14, 2017

Currently Phoenicis reads a json file which contains all that data. The thing is that this should be done in JS (like it is currently) because we want to support other engines than Wine. Would be could to run a profiler to check what's the deal before starting any fixes.

@madoar
Copy link
Collaborator

madoar commented Aug 14, 2017

And where is the json file managed, i.e. how is it created and updated, when a new wine version is released?
We could add a new function or class to the engine types that provides a getAllAvailableEngineVersion method. This method could then be called inside EngineManager to fetch all available engine version.

@plata
Copy link
Collaborator

plata commented Aug 14, 2017

That's the point in #973: It's available online and updated by @qparis. However, this is a different issue. I guess your suggestion should work. The EngineManager should register with the RepositoryManager to be notified about repository updates. Then, it should query all available engines in the repository for the available versions.
If we would distinguish engines and applications in the DTO tree directly, we wouldn't even need an EngineManager.

@madoar
Copy link
Collaborator

madoar commented Aug 14, 2017

How would a clear partition in engines and applications in the DTO tree remove the need of an EngineManager?

@plata
Copy link
Collaborator

plata commented Aug 14, 2017

I believe that the information we need could be stored in the EngineCategoryDTO directly. Not sure if we want that though.

@madoar
Copy link
Collaborator

madoar commented Aug 14, 2017

I think you could add the method to select all available engine versions to the EngineCategoryDTO, but I don't think that it can supply all functionality that would be included in an EngineManager class.
The difference is, that an EngineCategoryDTO contains only information about a single category of engines, e.g. wine, right?
But the EngineManager would contain information about all known engine categories.

@plata
Copy link
Collaborator

plata commented Aug 15, 2017

Yes. My idea was that all known engine categories are available from the repository. But it probably makes sense to have a manager anyways.

@plata
Copy link
Collaborator

plata commented Aug 16, 2017

I just did some profiling: 70% of the load when opening a container is caused by Downloader#saveConnectionToStream.

@madoar
Copy link
Collaborator

madoar commented Aug 16, 2017

Why/when do we use a Downloader when showing a container view?

@plata
Copy link
Collaborator

plata commented Aug 16, 2017

Like you said: Wine.prototype.getAvailableVersions from EnginesSource.

@madoar
Copy link
Collaborator

madoar commented Aug 16, 2017

Ok.
Let's solve this in two PRs:

  1. we deactivate the ComboBox, because it has no other functionality except showing which wine versions are available
  2. we replace the ComboBox with a new wizard dialog/view, which shows a window similar to the engine view, where the user has the ability to first see all available engines and second to switch to another engine.

What do you think?

@plata
Copy link
Collaborator

plata commented Aug 16, 2017

What about your idea of enhancing the EnginesSource to an EnginesManager which fetches the list only if the repository changes?

@plata
Copy link
Collaborator

plata commented Aug 16, 2017

I have implemented a solution in #979 and PhoenicisOrg/scripts#328. However, it does not work currently because the server does not return a proper date for the last file modification. If this can be fixed, this solution will only download a new file with available versions if really required.

@madoar
Copy link
Collaborator

madoar commented Aug 19, 2017

How should we do the remaining two PRs, I've suggested?
The first one I can today.
The second one, which includes a new engine update wizard, will take longer and requires an "engine update" method, which doesn't exist yet. Therefore I would suggest to move this second PR to the beta version.
What do you think?

@plata
Copy link
Collaborator

plata commented Aug 19, 2017

Probably the best approach. Can you open a new issue for the second point?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants