Yad gui speed improvements #2370
Conversation
|
I am currently out of town. Will take a look mid/late next week. Edit: marking as draft just so I don't accidentally click the merge button on mobile. |
|
@Botspot i checked it all out. I noticed such a bug, I attach a video. If I write the text, you will not be able to understand me.
|
|
Previously, an additional window was tied to the menu of the list of programs (although it was not taken into account that this could go outside the screen). Now as I understand it, you have made a reference to a certain place on the screen.
I write about the fact that I cannot control the position of the pop-up menu, which can close other windows. |
|
I managed to make two windows, but it's hard to replicate. I have an ssd, pi-apps manage to open and close all windows quickly. |
|
The main issue uncovered by Rak1ta is that this implementation breaks how the |
I have solved this issue. I am still looking for a way to prevent clicks from stacking up when clicking the Search button multiple times. |
|
@Botspot the major issue I forsee with this has to do with the update to this change. I have not tested these changes but I know that on update, the old GUI script will be running. Clicking on any app/settings/etc will call the gui script with the 3rd argument which functionality you have removed. this will either result in the wrong thing executing or nothing happening at all. If we want to keep these changes, I think we will need to bump |
gui
Outdated
| if [ "$1" != 0 ];then | ||
| sleep 1.5 | ||
| fi | ||
| pkill -P "$detailspid" 2>/dev/null |
There was a problem hiding this comment.
this kills child PIDs of the parent process (the backgrounded details_window function)
this (I think) will kill any running or backgrounded terminal_manage calls spawned from that detailspid
There was a problem hiding this comment.
I had not considered this possibility, but it does not seem to kill the terminal in practice. Not sure why not.
There was a problem hiding this comment.
I think in practice this is impossible to hit because in order to kill the detailspid that has the manage child process, you would need to click uninstall/install and another app name in the list at the exact same time.
Since the partent process (the details window) pid disappears once the yad details window closes on clicking the install/uninstall button, the kill_details pkill does nothing since that pid does not exist anymore (even though it has child PIDs, eg: the manage daemon, still active).
|
@theofficialgman I think it was unintentional to push a snapdrop commit to this branch. |
theofficialgman
force-pushed
the
gui-speedup
branch
from
71519a3 to
646b2e1
Compare
what snapdrop commit 😆 ? yes it was unintentional. thought I had master checked out |
I just tried simulating an update from old to new. When I clicked on a category in the old gui, the new gui launched, covering the old with an identical app list. From there it worked fine. I'm inclined to leave it like this - it seems less unexpected than having |
|
After thinking about it more, I went ahead with GUI_FORMAT_VERSION=3. |
theofficialgman
force-pushed
the
gui-speedup
branch
from
d01865f to
1fe915e
Compare
hmm... the old GUI did not close. something changed to make this runonce not affective anymore. it did execute, but it didn't close the GUI edit: yeah so the gui process gets killed (the script gui) but the yad window remains and is non-functional. |
|
I found a way to kill the yad windows (by using the knowledge we have of what arguments it was started with to get the pid from ps). this kills both the old gui script and the old yad window, however the new yad window pops up in the top left with a strange size and it closes after 30 seconds (the timer from the terminal-manage script that I guess the new gui got spawned as a child PID from) |
|
The benifits of these changes are clear. However I don't think there is a way to integrate them without asking the user to re-start pi-apps. We can do that easily by adding an if statement near the top of the GUI script that checks the I tried |
|
also I'm going to be honest I don't see any speed differences with this PR compared to master (both are instant for me). If you see any that must mean you are pretty IO limited (on your storage). Since the only difference is this PR has all of the functions already in bash, while master has to read from storage (or a cache) and execute the gui script again. |
|
I've tried every iteration of forking (&), disown, nohup, setsid. everything either dies when the timer ends (still a child process) or fails to run terminal-run (like mentioned before because the process gets sent to init and that can't run basically any GUI stuff) |
|
outside of the outright removal of the mange, api, settings, or other fundamental scripts I see no reason why we would ever need to close or restart the GUI on the user ever again from now on. Since the GUI is a self contained running script now (it does not call itself) an update can not cause a break in functionality. |
|
I found a better way to transition to the new gui, hold on... |
|
Took a WHILE but I figured out the issue stems from the how yad is calling the commands. yad was directly calling another yad window before this PR. now the gui script is calling the secondary yad commands. yad doesn't like that and won't raise if you do that. the while loop is unnecessary, so I got rid of it :) |
|
ah crap this breaks killing the windows ofc... |
It also introduces possible issues if multiple action threads are running concurrently. A while loop has more control over what is being run, and when, and can easily pass information forward from previous iterations. I'm hoping you can find another way to fix the window focusing issue that only seems to be happening on your system. |
Two systems. Different hardware. Different DEs. Different OS. Make sure to test yourself from the . desktop file since it doesn't happen when running pi-apps/gui from terminal. I have yet to test on piOS buster/bullseye and will do tonight but I bet it happens there as well. |
|
@Botspot before the PR, before dc1fa20 , and after dc1fa20 on piOS bullseye with the default DE (64bit on a pi3), the updater window is not focused on launch. So this DE does not work regardless of what we do and always has this issue #2370 (comment) |
the gui functioned this way before the PR and there were no issues. |
|
@Botspot I have tested more systems. in summary has the issue (updater not focused on launch) before the PR: has the issue (updater not focused on launch) with the PR but before my fix (dc1fa20): has the issue (updater not focused on launch) with the PR but after my fix (dc1fa20): So my fix corrects all DEs (3) that have the bug as a result of this change and does nothing for the broken DEs (piOS) that never properly raised the window in the first place |
no affect. |
solved with 212e539 |
theofficialgman
force-pushed
the
gui-speedup
branch
from
4203e74 to
83b5b6c
Compare
theofficialgman
force-pushed
the
gui-speedup
branch
from
83b5b6c to
47272a7
Compare
relaunch gui on update
if the user is running GUI_FORMAT_VERSION unset or 1, then they have the old updater that was not part of the manage daemon. so spawning a new GUI will not be part of the manage daemon and will succeed. if the user is on GUI_FORMAT_VERSION 2 then the likelyhood that they have the new manage daemon updater is high so we can not simply respawn the GUI since it would be a child process of the manage daemon (and would be killed after the 30 second timeout). So we are forced to detect this and kill the GUI process and yad window and tell the user to restart pi-apps themselves.
`app_search_gui` and details_window are now part of the same `detailspid`. so kill_details to kill any GUI window spawned by these functions. In addition, `app_search_gui` is no longer blocking so `kill_details` can kill it if the user slicks search again.
yad now calls the `run_user_selection` function directly which allows for DEs to properly handle window focusing. also modify the parsing of yad output in app_search_gui since that strangely changed as a side affect of this change workaround array variables not being passed to `bash -c` bash -c specifically does not pass the contents of array variables. we use an array variable to set the default yadflags and it is the only variable that need to be set that we don't have access to inside bash -c without this the spawned yad windows from app_search_gui, the details window, and updater will be missing the default flags this also affects the settings yad GUI which spawns other scripts with bash -c, however all of those scripts don't rely on the yadflags (or re-set them internally) so the issue doesn't occur there
`kill_details` is a blocking function and part of it is sent to the background so as to not block further execution. To prevent accidentally reading the new detailspid that gets added to the list, make sure to read the current contents first in a blocking manner and then send the kill loop to the background.
theofficialgman
force-pushed
the
gui-speedup
branch
from
47272a7 to
4ba32ef
Compare
|
@Botspot I really would have preferred that do a rebase. It's impossible to have issues if you do one since you get forced to resolve conflicts manually during the process. Using github actions with vscode as the editor its a breeze to resolve any |
I actually was forced to resolve conflicts, but I did it incorrectly. I should have tested gui after resolving the conflicts, not before. |
|
fyi, the github actions check should have been an indication for you not to merge you are welcome to add a rule to prevent merging unless all checks pass. I can't do that since I don't have permissions on this repo to edit settings. https://stackoverflow.com/questions/60776412/github-actions-is-there-a-way-to-make-it-mandatory-for-pull-request-to-merge |
Avoids running a gui script subprocess every time a category or app is clicked. This has long been the behavior of the xlunch mode but I've had plans to extend it to yad. My past attempts failed but this time around it looks pretty solid. The interface is noticeably smoother and more snappy. I have not benchmarked it.
The other major benefit from these changes is that the app details window no longer vanishes when you try to drag it around the screen, resize it, or when you bring another window into focus. The details window will now close only when the user begins to do something else.
I have completed basic testing of the interface and have corrected all bugs found. It is entirely possible that edge cases have gone untested. While debugging, I found a lot of cases where a dialog (app list, details window, search) was supposed to close but no longer does, so I would not be too surprised if there are a couple more cases where that particular issue crops up.
I'm interested in feedback from @theofficialgman and maybe some testing from @Rak1ta if possible. (Rak1ta you seem to find a lot of bugs!)