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

Reformat version number in Software Update screen to match brave://settings/help and brave://version #13819

Closed
isdn opened this issue Jan 28, 2021 · 17 comments · Fixed by brave/brave-core#8642

Comments

@isdn
Copy link

isdn commented Jan 28, 2021

Description

Please see the following screenshot.

Screen Shot 2021-01-28 at 21 45 12

According to brave://settings/help, my current version is Version 1.19.86 Chromium: 88.0.4324.96 (Official Build) (x86_64) and the new version is 1.19.88.

OS version: macOS Big Sur 11.1.

@bsclifton
Copy link
Member

Ran this by the release team - this unusual version 119.88 is expected; it's the Sparkle (updater used for macOS) "short version". It's concatenating the MAJOR + MINOR together unfortunately

@bsclifton bsclifton changed the title Software update popup - incorrect versions update popup - incorrect version shown (shows Sparkle version - ex: 119.88 vs 1.19.88) Jan 28, 2021
@bsclifton
Copy link
Member

See brave/Sparkle#14

@mherrmann mherrmann self-assigned this Apr 23, 2021
@rebron rebron added the priority/P3 The next thing for us to work on. It'll ride the trains. label Apr 23, 2021
@rebron rebron added priority/P4 Planned work. We expect to get to it "soon". and removed priority/P3 The next thing for us to work on. It'll ride the trains. labels Apr 23, 2021
@mherrmann
Copy link

@bsclifton @mihaiplesa in light of #9562, should this issue be fixed at all? Should the fix not maybe be to get rid of the update popup entirely?

@mihaiplesa
Copy link
Contributor

Haven't checked newer Sparkle releases, but is there an option for that?

@mherrmann
Copy link

Also after reading around a little, I'm not sure whether the popup appears because of a bug in Sparkle or because of a bug in Brave's sparkle integration.

In both cases, I don't think the popup should be shown when the user goes to brave://settings/help. After all, there already is visual feedback about the availability of an update on that page. Showing a popup on top of that does not make sense in my eyes.

However, there might be other cases where the popup is wanted. For example, maybe the popup should be shown if the user just keeps Brave open for weeks and should benefit from the update for security reasons.

Given that I already completed the work for this present issue in brave/brave-core#8642 and https://github.com/brave/omaha-server-private/pull/64, I think my preference would be to get this done and leave #9562 as a follow-up. The changes in the two PRs make the implementation cleaner (in my opinion) in any case.

@mherrmann
Copy link

@mihaiplesa @bsclifton just a quick ping so we don't forget about this issue. How do you think we should continue?

  1. Merge the two PRs Fix version shown in auto-update popup on Mac brave-core#8642 and brave/omaha-server-private#64, leaving Software update popup should not be presented during Sparkle update process #9562 as a follow-up.
  2. Throw away the PRs and just do Software update popup should not be presented during Sparkle update process #9562.

As I explained in my comment above, I think 1) is better but it involves small changes (and thus a release) on the server side.

@mihaiplesa
Copy link
Contributor

@rebron thoughts - do we want pop-ups? Ideally we'd like to update in the background like on the other platforms.

@mherrmann
Copy link

Can I briefly again ping you about this too @rebron?

@rebron
Copy link
Collaborator

rebron commented Jun 8, 2021

cc: @kliu

@rebron
Copy link
Collaborator

rebron commented Jun 11, 2021

@mherrmann We don't want to show the pop-up but let's go ahead and proceed with what you proposed.

Merge the two PRs brave/brave-core#8642 and brave/omaha-server-private#64, leaving #9562 as a follow-up.

@kjozwiak
Copy link
Member

@brave/legacy_qa we'll need two RC's that have the above fix to verify this properly. You won't be able to update from 1.25.73 to the new RC. You need two builds that include the above fix. Example:

  • install the 1.26.x RC1 pkg (includes the above fix)
  • update to 1.26.x RC2 (includes the above fix)

@LaurenWags
Copy link
Member

LaurenWags commented Jun 21, 2021

Verified passed on macOS x64 Catalina 10.15.7 with

Brave | 1.26.64 Chromium: 91.0.4472.114 (Official Build) (x86_64)
-- | --
Revision | 4bb19460e8d88c3446b360b0df8fd991fee49c0b-refs/branch-heads/4472@{#1496}
OS | macOS Version 10.15.7 (Build 19H1030)

Verified changes to version display on update popup.
Reproduced the issue using 1.25.73 --> 1.26.65 update.
Confirmed versions display nicer using 1.26.64 --> 1.26.65 update.

1.25.73 --> 1.26.65 1.26.64 --> 1.26.65
1 25 73 update popup 1 26 64 update popup

Verified passed on macOS arm64 Big Sur 11.4 with

Brave	1.26.64 Chromium: 91.0.4472.114 (Official Build) (arm64)
Revision	4bb19460e8d88c3446b360b0df8fd991fee49c0b-refs/branch-heads/4472@{#1496}
OS	macOS Version 11.4 (Build 20F71)

Verified changes to version display on update popup.
Reproduced the issue using 1.25.73 --> 1.26.65 update.
Confirmed versions display nicer using 1.26.64 --> 1.26.65 update.

1.25.73 --> 1.26.65 1.26.64 --> 1.26.65
1 25 73 1 26 64

Verified passed on macOS-Intel Big Sur 11.4 with

Brave	1.26.64 Chromium: 91.0.4472.114 (Official Build) (arm64)
Revision	4bb19460e8d88c3446b360b0df8fd991fee49c0b-refs/branch-heads/4472@{#1496}
OS	macOS Version 11.4 (Build 20F71)

Verified changes to version display on update popup.
Reproduced the issue using 1.25.73 --> 1.26.65 update.
Confirmed versions display nicer using 1.26.64 --> 1.26.65 update.

1.25.73 --> 1.26.65 1.26.64 --> 1.26.65
Screen Shot 2021-06-21 at 9 56 09 AM Screen Shot 2021-06-21 at 10 00 49 AM

@rebron rebron changed the title update popup - incorrect version shown (shows Sparkle version - ex: 119.88 vs 1.19.88) Reformat version number in Software Update screen to match brave://settings/help and brave://version Jun 21, 2021
@bsclifton
Copy link
Member

Reverted by @mihaiplesa on all channels after experiencing #16539
master - brave/brave-core@14e74d6
1.27 - brave/brave-core@47f369b
1.26 - brave/brave-core@7f3c11a

@bsclifton bsclifton reopened this Jun 22, 2021
@LaurenWags
Copy link
Member

Removed from 1.26.x milestone since this was reverted

@LaurenWags LaurenWags removed this from the 1.26.x - Release milestone Jun 22, 2021
@mherrmann
Copy link

In light of the above problems and the revert (sorry again and thank you @bsclifton and @mihaiplesa for the quick fix), I suggest we forego this issue and just do #9562.

@bsclifton
Copy link
Member

@mherrmann all credit to @mihaiplesa 😄 I'll go ahead and close this then as wontfix

@mherrmann
Copy link

Well, then thank you again @mihaiplesa ❤️

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

Successfully merging a pull request may close this issue.

8 participants