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

PR: Improve update UX and automatically download latest installers #18619

Merged
merged 50 commits into from
Sep 19, 2022

Conversation

jsbautista
Copy link
Contributor

@jsbautista jsbautista commented Jul 11, 2022

Description of Changes

  • Show download status and status bar icon (Spyder version, if checking for updates, if an update is pending, downloading installer, installing, etc)
  • Logic to download and run the new Windows/MacOS installer version if available

Part of #19112

Affirmation

By submitting this Pull Request or typing my (user)name below,
I affirm the Developer Certificate of Origin
with respect to all commits and content included in this PR,
and understand I am releasing the same under Spyder's MIT (Expat) license.

I certify the above statement is true and correct: @jsbautista

@pep8speaks
Copy link

pep8speaks commented Jul 11, 2022

Hello @jsbautista! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2022-09-09 15:07:58 UTC

@dalthviz dalthviz changed the title PR: Changes on check updates WIP [WIP] PR: Changes on check updates Jul 11, 2022
@dalthviz dalthviz added this to the v5.3.3 milestone Jul 11, 2022
@jsbautista
Copy link
Contributor Author

@dalthviz ; A preview of the process of checking for new updates:
Preview

@dalthviz
Copy link
Member

@jsbautista just in case, I think the tests are failling since for the new application/widgets dir you need to add a __init__.py file.

@dalthviz
Copy link
Member

Seems like there are some errors with the status constants @jsbautista. To check the tracebacks you can check the CI log: https://github.com/spyder-ide/spyder/runs/7567303687?check_suite_focus=true#step:15:1790

@dalthviz
Copy link
Member

dalthviz commented Aug 2, 2022

@jsbautista I think that this needs a rebase on top of the latest 5.x to prevent errors in the CI. For that fetch in your local repo upstream and then rebase on top upstream/5.x. So running something like this should do it:

git checkout 5.xUpdateSpyder
git fetch upstream
git rebase upstream/5.x

If that works without issue the you can force push the rebase with:

git push -f

If you need help or some error message appears let me know 👍

@jsbautista
Copy link
Contributor Author

Hi @dalthviz, could you give me a last review before removing the WIP?

Copy link
Member

@dalthviz dalthviz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @jsbautista thanks for all the work on this! I left some suggestions and comments. I think the most important part to check is the location of the installation thread logic. Checking the code a little bit more seems to me that the status of the installation should be handle by the installation widget and we need to provide methods so the container can call some start method using the status widget (so we have a relation like container -> status widget -> installation widget).

If you have any questions or comments maybe we can do a pair programming session to test the ideas or check if there are more elements to take into account to handle better the interactions between container, status widget and installation widget.

Let me know!

spyder/plugins/application/widgets/status.py Outdated Show resolved Hide resolved
spyder/plugins/application/widgets/install.py Outdated Show resolved Hide resolved
spyder/plugins/application/widgets/status.py Outdated Show resolved Hide resolved
spyder/plugins/application/container.py Outdated Show resolved Hide resolved
spyder/plugins/application/container.py Outdated Show resolved Hide resolved
spyder/plugins/application/widgets/status.py Outdated Show resolved Hide resolved
spyder/plugins/application/widgets/status.py Outdated Show resolved Hide resolved
spyder/plugins/application/widgets/install.py Outdated Show resolved Hide resolved
spyder/plugins/application/widgets/install.py Outdated Show resolved Hide resolved
spyder/plugins/application/widgets/install.py Outdated Show resolved Hide resolved
@jsbautista
Copy link
Contributor Author

Hi @dalthviz

Copy link
Member

@dalthviz dalthviz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all the work @jsbautista ! I think this is getting closer and closer to be finished 👍

I left some suggestions regarding the calls to set the status and also some remaning logic position and status values suggestions (like using the Spyder version for the NO_STATUS constant). Besides that there are some PEP8 issue to be fixed.

Also, could you update the gif to preview the way this is working, please?

Thanks again! If you need anything to address the review, questions or comments, let me know!

spyder/plugins/application/container.py Outdated Show resolved Hide resolved
spyder/plugins/application/container.py Outdated Show resolved Hide resolved
spyder/plugins/application/container.py Outdated Show resolved Hide resolved
spyder/plugins/application/widgets/install.py Outdated Show resolved Hide resolved
spyder/plugins/application/widgets/install.py Outdated Show resolved Hide resolved
spyder/plugins/application/widgets/install.py Outdated Show resolved Hide resolved
spyder/plugins/application/widgets/install.py Outdated Show resolved Hide resolved
spyder/plugins/application/widgets/install.py Outdated Show resolved Hide resolved
spyder/plugins/application/widgets/install.py Outdated Show resolved Hide resolved
spyder/plugins/application/widgets/status.py Outdated Show resolved Hide resolved
@jsbautista
Copy link
Contributor Author

@dalthviz ; A preview of the process of checking for new updates:
UpdateSpyder

@jsbautista jsbautista changed the title [WIP] PR: Changes on check updates PR: Changes on check updates Aug 12, 2022
Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work @jsbautista! This looks pretty good!! Most of my suggestions are style issues, otherwise looks almost ready to me.

As a recommendation, please don't mark reviewer comments as solved. That's the work of reviewers, after checking that you correctly addressed their comments.

spyder/plugins/application/container.py Show resolved Hide resolved
spyder/plugins/application/container.py Outdated Show resolved Hide resolved
spyder/plugins/application/container.py Outdated Show resolved Hide resolved
spyder/plugins/application/container.py Show resolved Hide resolved
spyder/plugins/application/container.py Outdated Show resolved Hide resolved
spyder/plugins/application/widgets/install.py Outdated Show resolved Hide resolved
spyder/plugins/application/widgets/status.py Show resolved Hide resolved
spyder/plugins/application/widgets/status.py Outdated Show resolved Hide resolved
spyder/plugins/application/widgets/status.py Outdated Show resolved Hide resolved
spyder/plugins/application/widgets/status.py Outdated Show resolved Hide resolved
Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jsbautista, last comments from my side.

spyder/plugins/application/widgets/install.py Outdated Show resolved Hide resolved
spyder/plugins/application/widgets/install.py Outdated Show resolved Hide resolved
@CAM-Gerlach
Copy link
Member

I've made a suggestion to fix the description messages, FYI

Co-authored-by: Daniel Althviz Moré <d.althviz10@uniandes.edu.co>
@CAM-Gerlach
Copy link
Member

Also, just to confirm @dalthviz @ccordoba12 once this is ready, this should either have a major rebase to consolidate the many "WIP" commits into atomic, well-described units before merging, or just be squash merged.

jsbautista and others added 2 commits September 1, 2022 14:44
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
@dalthviz
Copy link
Member

dalthviz commented Sep 2, 2022

@CAM-Gerlach this will be squash merged 👍

Copy link
Member

@dalthviz dalthviz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @jsbautista ! Left some comments regarding some issues raising when testing the latest changes.

Also, just in case, a preview for others to see how this will be working now to reuse a previously downloaded installer executable:

  • The installer executable was downloaded in a previous session but not installed and the check update dialog appears:

initial

  • In case the installer was downloaded in the current session or a previous one but the user decided to not proceed with the installation update (in case of the Windows installer the user choosed to not close Spyder for example or choosed No in the check update dialog):

continue

spyder/plugins/application/container.py Outdated Show resolved Hide resolved
reply.setStandardButtons(QMessageBox.Yes | QMessageBox.No)
reply.exec_()
if reply.result() == QMessageBox.Yes:
self.start_installation_update()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is triggering an TypeError exception since the latest_release_version arg is not being passed:

Traceback (most recent call last):
  File "C:\Users\dalth\AppData\Local\Programs\Spyder\pkgs\spyder\plugins\application\widgets\status.py", line 111, in show_installation_dialog_or_menu
    self.installer.continue_install()
  File "C:\Users\dalth\AppData\Local\Programs\Spyder\pkgs\spyder\plugins\application\widgets\install.py", line 191, in continue_install
    self.start_installation_update()
TypeError: start_installation_update() missing 1 required positional argument: 'latest_release_version'

I think we should, from the container, pass the latest_release_version to the status widget/installer widget even if the user doesn't accept immediatly the first dialog to update (the one that informs that a new version is available)

jsbautista and others added 3 commits September 6, 2022 13:56
Co-authored-by: Daniel Althviz Moré <d.althviz10@uniandes.edu.co>
jsbautista and others added 4 commits September 9, 2022 07:56
Co-authored-by: Daniel Althviz Moré <d.althviz10@uniandes.edu.co>
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
@dalthviz
Copy link
Member

dalthviz commented Sep 9, 2022

Just in case @jsbautista seems like the error to generated the Windows installer is unrelated to the changes on the PR but to the PyPI json API cache to retrieve a release/info about the certifi package which had a release some hours ago.

Copy link
Member

@dalthviz dalthviz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all the changes @jsbautista ! I left a simple comment for the latest changes regarding a variable name.

Also, as a suggestion we were discussing with the others, we should leave the update related status bar widget to the left of the rest of status bar widgets. Checking, for that we will need to add some entries in a couple of iterables in the status plugin. Basically, we need change the set and list at:

INTERNAL_WIDGETS_IDS = {
'clock_status', 'cpu_status', 'memory_status', 'read_write_status',
'eol_status', 'encoding_status', 'cursor_position_status',
'vcs_status', 'lsp_status', 'kite_status', 'completion_status'}

And

internal_layout = [
'clock_status', 'cpu_status', 'memory_status', 'read_write_status',
'eol_status', 'encoding_status', 'cursor_position_status',
'vcs_status', 'lsp_status', 'kite_status', 'completion_status']

Adding to them the following entries: 'interpreter_status', 'application_update_status'

With that the status widget for the updates will be shown at the left of the others:

imagen

If you have any question or comment let me know!

spyder/plugins/application/widgets/install.py Outdated Show resolved Hide resolved
jsbautista and others added 2 commits September 13, 2022 17:24
Co-authored-by: Daniel Althviz Moré <d.althviz10@uniandes.edu.co>
Copy link
Member

@dalthviz dalthviz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jsbautista ! I think this is ready for a squash merge!

Do you have any other comments @mrclary @CAM-Gerlach @ccordoba12 ?

@mrclary
Copy link
Contributor

mrclary commented Sep 14, 2022

I'm good

Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to look good from a UX and UI text perspective, with some improvements to follow in a followup PR. I haven't had the time to fully QA test it myself on a Windows machine, but it seems others have.

Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jsbautista for your hard work on adding this new functionality! It's a terrific improvement!!

@dalthviz dalthviz merged commit 14f658b into spyder-ide:5.x Sep 19, 2022
dalthviz added a commit that referenced this pull request Sep 19, 2022
PR: Improve update UX and automatically download latest installers
dalthviz added a commit that referenced this pull request Sep 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants