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

use manage daemon for GUI updates #2340

Merged
merged 29 commits into from
Jun 7, 2023
Merged

use manage daemon for GUI updates #2340

merged 29 commits into from
Jun 7, 2023

Conversation

theofficialgman
Copy link
Collaborator

@theofficialgman theofficialgman commented May 30, 2023

updates for apps now occur in the GUI manage daemon just like app installs and uninstalls. it is now safe for the user to start updates and then continue to install/uninstall apps during the update process since they all share the same manage daemon.

the onboot updater now also uses the manage daemon as a result and users can start updates from the onboot updater and then open pi-apps and immediately start installing/uninstalling applications

updater Outdated Show resolved Hide resolved
@theofficialgman
Copy link
Collaborator Author

theofficialgman commented May 30, 2023

fyi this is not complete yet, I still have plans of improving the update function by

  • making the manage-daemon exection order deterministic. currently the order appears to get randomized or changed after the first execution. this was the case prior to these changes already.
  • make "Inactive updates" appear differently in the manage-daemon (now called "Refresh" in the GUI)
    • have "Inactive updates" automatically move the top of manage-daemon execution. This way any apps the user queues up always are the latest version if there are updates already queued.
      • Update check for user installing out of date app to not prompt if the app is currently scheduled for a refresh in the manage daemon
  • have files update queue in the manage daemon to prevent overwriting files while another app might be installing/uninstalling
    • as part of this, files update queue should move to the top of manage-daemon execution above "Inactive updates"

manage Outdated Show resolved Hide resolved
manage Outdated Show resolved Hide resolved
updater Show resolved Hide resolved
icons/refresh.png Outdated Show resolved Hide resolved
@theofficialgman
Copy link
Collaborator Author

@Botspot I am running into an issue with attempting to implement this:

Update check for user installing out of date app to not prompt if the app is currently scheduled for a refresh in the manage daemon

The problem is that validate_apps_gui is run on the app as the user selects it, not when it actually gets installed by the manage-daemon. The instance of the manage-daemon where the user selects the app to install has no way of knowing what is currently in the queue. The user could already have the app refresh in the queue, in which case they shouldn't be prompted to update.

There isn't really any harm though in not handeling this edge case, there will just be duplicate refresh and update entries in the manage-daemon when the user inevitably selects the app to update again in the update GUI. I could solve the duplicate entries by simply removing them as part of the reorder_list function if we wanted to.

What are your thoughts?

@Botspot
Copy link
Owner

Botspot commented Jun 1, 2023

I've been monitoring this PR and reading the code bit by bit in my spare time. (I'm starting a business and taking an accelerated summer course so my time is short)
I can probably do a full review tomorrow afternoon.

@theofficialgman theofficialgman force-pushed the updater-manage branch 2 times, most recently from f2b739c to f861f5a Compare June 1, 2023 19:23
@theofficialgman
Copy link
Collaborator Author

Update check for user installing out of date app to not prompt if the app is currently scheduled for a refresh in the manage daemon

regarding this, I think its OK if it isn't implemented at all. it is an edge case that will be difficult for a user to hit and doesn't have any lasting negative impacts if they hit it.

Since app refreshes are prioritized they get moved to the top of the manage-daemon list. It is unlikely that the user would have any pending app refreshes when they go to install an app since each app refresh takes less than a second. Basically an app refresh for an app the user chose to may be in the queue when they go to install that app, but it will almost always already be complete in the queue, so the out of date check will not flag the app as out of date.

@theofficialgman theofficialgman marked this pull request as ready for review June 1, 2023 20:28
@theofficialgman
Copy link
Collaborator Author

This PR is finalized from me now unless anyone requests changes.

manage Outdated Show resolved Hide resolved
updater Outdated Show resolved Hide resolved
api Show resolved Hide resolved
api Show resolved Hide resolved
api Outdated Show resolved Hide resolved
updater Outdated Show resolved Hide resolved
@Botspot
Copy link
Owner

Botspot commented Jun 3, 2023

I like the majority of this PR but did not like the dissonance between the updater's preview of individual files but the manage daemon's lumping them altogether. I have changed manage-daemon to display each updatable file on a separate line, with an informative icon based on the minetype of the file (to match what updater does), while removing your introduction of a new delimiter your filelist approach.
I also disliked how the manage "update" mode was being hijacked to update a list of files: I have always intended for manage to be a close approximation to dpkg for debian, and this worked against that ideal.

Now a new action type is introduced for updating files: update_file. When this action is encountered by manage daemon, it will directly run update_now_cli instead of launching a subprocess of manage.

It is late here and I was not able to test any of these changes yet. But they seem to be fairly trivial and I doubt it will take me long to do it later.

@theofficialgman
Copy link
Collaborator Author

Now a new action type is introduced for updating files: update_file. When this action is encountered by manage daemon, it will directly run update_now_cli instead of launching a subprocess of manage.

The reason I "hijacked" update was so that I didn't have to add another action type. These are still updates, just file updates, and the action type was used to set the name that gets written to the GUI (updating, updated, etc)

updater Outdated Show resolved Hide resolved
manage Outdated Show resolved Hide resolved
manage Outdated Show resolved Hide resolved
@theofficialgman
Copy link
Collaborator Author

I like the majority of this PR but did not like the dissonance between the updater's preview of individual files but the manage daemon's lumping them altogether. I have changed manage-daemon to display each updatable file on a separate line, with an informative icon based on the minetype of the file (to match what updater does), while removing your introduction of a new delimiter your filelist approach. I also disliked how the manage "update" mode was being hijacked to update a list of files: I have always intended for manage to be a close approximation to dpkg for debian, and this worked against that ideal.

It was significantly better before. now the state of the PR is broken. I could have easily made the files appear as multiple lines in the YAD UI while still have them as a filelist so that they could be updated at once if you had asked. Dropping your commits to do that since this is too broken

manage Outdated Show resolved Hide resolved
@theofficialgman
Copy link
Collaborator Author

theofficialgman commented Jun 3, 2023

It was significantly better before. now the state of the PR is broken. I could have easily made the files appear as multiple lines in the YAD UI while still have them as a filelist so that they could be updated at once if you had asked. Dropping your commits to do that since this is too broken

and now I have done this. easy 6a0d08c

theofficialgman and others added 11 commits June 7, 2023 18:49
app updates that reside in the compressed update now execute a "refresh" action instead of an "update" action. this allows us to treat them differently in the manage script and show differently in the manage-daemon GUI

add new refresh icon which denotes this as more of an action being completed instead of a function for the user to activate
necessary for future changes to allow for changing the entire queue order.  the previous for loop operating on new_lines would prevent re-ordering the entire queue
also correct `update_now_cli` which had an return status of 1 when `no_status` was true

filelist check is inside validate_apps_gui. this allows for multiple lines of input to be passed to manage daemon mode even though currently pi-apps does not do this

reorder queue list so app update filelists are prioritized
lsb_release can be slow to return (up to 0.1seconds). it is faster to call it once and obtain all the information necessary and then map it to the proper variables then to call it multiple times. these changes improve the speed from ~0.8 seconds runtime to ~0.3 seconds runtime. in addition, if the variables are already present, the call is skipped entirely.
I've been meaning to do this for a while. All pi-apps core scripts should use it except for terminal-run and viewlog.
`is_supported_system` takes up to 0.25 seconds to return and prevents fast execution of the manage script
The first action in the queue gets executed immediately when the manage-daemon first starts. Before these changes, the first action in the queue could be a refresh or an update depending on alphabetical order. Now the order of actions from the updater will always be files -> refreshes -> updates
have `terminal_manage` function as a legacy wrapper to `terminal_manage_multi`. this allows for pi-apps scripts (even old ones) to still call `terminal_manage` with two arguments. this is very important for the initial update where the old GUI is still running and the new manage script will be called.
sends all updates as part of one command. less threads spawned and overall faster execution
@theofficialgman theofficialgman force-pushed the updater-manage branch 2 times, most recently from fbbe8cb to 723936e Compare June 7, 2023 23:00
Botspot and others added 12 commits June 7, 2023 19:04
uses `terminal_manage_multi` to send all apps to the daemon.

Also corrects `refresh_list` to not hang if the `pipe` does not exist, like when the main pi-apps GUI is not open or does not use the same pipe as when `terminal_manage_multi` was spawned
avoid writing the queue to the manage daemon GUI after an install/uninstall/update/refresh has finished because the queue variable may be out of date. if I user added a new app to the queue while the daemon was busy performing an action on an app, then the new app the user added would disappear and then quickly re-appear when the loop was re-entered.
add a couple of optimizations/simplifications

Co-Authored-By: Botspot <54716352+Botspot@users.noreply.github.com>
manage: continue refreshing apps in daemon without updating GUI

skips timely GUI updates and queue pipe reads when performing sequential refresh actions
updater only gets sourced if the required function is not already present. the functions will always be present if run from the daemon. this is only for compatibility with running from CLI daemon where the updater script would not have already been sourced
by @Botspot 's logic this is out of the scope of user interaction in the manage script. We don't want users executing refresh on applications that they actually have installed since then they will miss any updates. Users should only call `update` which will automatically refresh as part of it.
@theofficialgman theofficialgman merged commit 3d4e1e9 into master Jun 7, 2023
4 checks passed
@theofficialgman
Copy link
Collaborator Author

merged

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 this pull request may close these issues.

None yet

2 participants