Make a single IPC call for the navigation history #6366
Conversation
absidue
requested review from
PikachuEXE,
efb4f5ff-1298-471a-8973-3d47447115dc,
ChunkyProgrammer,
kommunarr and
MarmadileManteater
github-actions
bot
added
the
PR: waiting for review
absidue
force-pushed
the
nav-history-perf
branch
from
8434aea to
c3a6332
Compare
There was a problem hiding this comment.
This is great! Thank you for doing this!
issue (minor): I am seeing a new bug that I can confirm I do not see in development: when making two consecutive page searches, the title displayed in the dropdown for the active entry is now incorrectly the title for the previous search, not the current one.
|
Okay this is an interesting one, as I can't spot any changes to the logic, I think it is actually a pre-exiting race-condition. Both the title setting code and the fetching the menu are async and I think by messing with one of them, I've changed the order in which they finish, changing the outcome. I'll try experimenting a bit with the title setting code, e.g. at the moment we call a store action that is just a thin wrapper around a store mutation, so if we directly call the mutation which is sync, instead of the action which is asnyc it might fix it. |
|
Fixed the issue. |
|
Im unsure if this is caused by the PR but will mention it anyway. The history will list the previous page for a split second when navigating to a route that has to load stuff e.g. watch page VirtualBoxVM_zdoBFIpFdv.mp4 |
|
I would say that that is expected behaviour as we don't know the video title until we have loaded the data, so we can't update the window title or the menu description until it has loaded. All we could do to improve the situation would be to add an extra in between title e.g. |
|
Should that be addressed in this PR or a separate one? |
|
I would say introducing a placeholder should happen in a follow up, as the main goal of this PR was to speed it up and there not being anything to show while it is loading is sort of unrelated to that, at least in my mind. |
github-actions
bot
removed
the
PR: waiting for review
* development: (57 commits) Translated using Weblate (Ukrainian) Translated using Weblate (Serbian) Translated using Weblate (Hungarian) Translated using Weblate (Icelandic) Translated using Weblate (Estonian) Translated using Weblate (Czech) Translated using Weblate (French) add missing release builds (FreeTubeApp#6415) Translated using Weblate (Serbian) Translated using Weblate (German) Translated using Weblate (Italian) Translated using Weblate (Chinese (Traditional Han script)) apply overflow menu text color change only to overflow menu (not to stats button) (FreeTubeApp#6406) Make a single IPC call for the navigation history (FreeTubeApp#6366) Added buttons to hide hidden channels/text (FreeTubeApp#6156) Translated using Weblate (Serbian) Translated using Weblate (Afrikaans) Translated using Weblate (Afrikaans) Translated using Weblate (Vietnamese) Translated using Weblate (Afrikaans) ...
Make a single IPC call for the navigation history
Pull Request Type
Related issue
Description
The current implemenation of the navigation history dropdowns makes a lot of IPC calls, one to get the active index, a second one to get the total number of history entries and then one IPC call for each history entry that it wants to get the title for. This pull request moves the construction of that navigation history list into the main process, that way we just need a single IPC call that can directly return the list, avoiding all of the overhead from the extra IPC calls.
While I was at it, I also replaced the IPC call to navigate to a history offset with
router.go(offset)as it does the same thing without needing to do the IPC call.Testing
Right click on the navigation arrows and navigate to various history entries, it should behave the same as before.
Desktop