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

Tracking version #966

Merged
merged 8 commits into from
Jun 18, 2021
Merged

Tracking version #966

merged 8 commits into from
Jun 18, 2021

Conversation

gera-gas
Copy link
Contributor

@gera-gas gera-gas commented Jun 9, 2021

New version check.
This pull request introduces an always-opt-in solution for checking a new Audacity version and further download updates by user actions. This feature will be only enabled if audacity_has_updates_check and audacity_has_networking is on.

Dialog example:
2021-06-07_16-43-48

Copy link
Collaborator

@Paul-Licameli Paul-Licameli left a comment

Choose a reason for hiding this comment

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

This PR has a too-complicated commit history, including merging of two branches with different upstream commits. The commits are not in one simple sequence.

This makes it too difficult to review, and to verify that each commit in the sequence builds and runs.

Please merge your work with recent master, then do
git reset audacity/master (or whatever remote name you have for master)
and then add and commit files again, so that there are fewer commits (maybe one). Then force-push it.

There should not be commits, in a final form of the branch, for small corrections like indentation.

There may be multiple commits to show how the project can be done in logical stages, to aid review. But the branch should not keep a history of correction of errors, unless you still mark it as a draft.

CMakeLists.txt Outdated Show resolved Hide resolved
@Paul-Licameli Paul-Licameli mentioned this pull request Jun 9, 2021
libraries/lib-update-version/UpdateManager.h Outdated Show resolved Hide resolved
libraries/lib-update-version/UpdatePopupDialog.h Outdated Show resolved Hide resolved
libraries/lib-update-version/UpdateManager.cpp Outdated Show resolved Hide resolved
libraries/lib-update-version/UpdateManager.cpp Outdated Show resolved Hide resolved
src/AudacityApp.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@Paul-Licameli Paul-Licameli left a comment

Choose a reason for hiding this comment

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

Further reconsideration of lifetime problems.

src/AudacityApp.h Outdated Show resolved Hide resolved
src/update/UpdateManager.h Outdated Show resolved Hide resolved
src/update/UpdateManager.cpp Outdated Show resolved Hide resolved
crsib
crsib previously requested changes Jun 9, 2021
src/update/UpdateManager.cpp Outdated Show resolved Hide resolved
src/update/UpdateManager.cpp Outdated Show resolved Hide resolved
src/update/UpdateManager.cpp Outdated Show resolved Hide resolved
src/update/UpdateManager.cpp Outdated Show resolved Hide resolved
@Paul-Licameli
Copy link
Collaborator

I will await resolution of points I raised before continuing review.

Copy link
Collaborator

@Paul-Licameli Paul-Licameli left a comment

Choose a reason for hiding this comment

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

I am satisfied that you addressed all points in my last review. Thank you for the simpler commit history.

This next review covers some points about naming and translation.

I have not reviewed everything yet.

src/update/UpdateManager.cpp Outdated Show resolved Hide resolved
src/update/UpdatePopupDialog.cpp Outdated Show resolved Hide resolved
src/update/UpdatePopupDialog.cpp Outdated Show resolved Hide resolved
src/update/UpdateManager.h Outdated Show resolved Hide resolved
Copy link
Collaborator

@Paul-Licameli Paul-Licameli left a comment

Choose a reason for hiding this comment

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

Some more points about parent window pointers and lifetimes.

src/update/UpdateManager.cpp Outdated Show resolved Hide resolved
src/update/UpdateManager.cpp Outdated Show resolved Hide resolved
@gera-gas gera-gas moved this from In progress to Review in Release 3.0.3 Jun 16, 2021
@Paul-Licameli
Copy link
Collaborator

Before anything else: again, please simplify the commit history. Make one simple, straight branch. Do not include merge commits. Rebase or cherry-pick one of the branches onto the other. You could do

git checkout tracking-version
git reset --hard 1426e5f2c021116ae61dd6e350db724a07e8529c
git cherry-pick 944edbcc0f3180d6aaf93ab9ef1247404eec64e5
git cherry-pick 14922cfc738df1bca1401f0a75a178dd35f7c40d

And then, rebase onto more recent master and force-push. You might also do git rebase -i and make some of these commits fixups so the history is shorter.

Copy link
Collaborator

@Paul-Licameli Paul-Licameli left a comment

Choose a reason for hiding this comment

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

The one incorrect static variable is the only mildly important thing in this review that isn't just style. It's wrong in principle, but even so isn't likely to cause an error in practice because of the long update interval, longer perhaps than most user sessions with Audacity.

Besides that, the commit history must be cleaned up, without merge commits, before I approve.

Overall I'm pleased with this branch.

src/prefs/ApplicationPrefs.cpp Outdated Show resolved Hide resolved
src/update/UpdateManager.cpp Outdated Show resolved Hide resolved
src/update/UpdatePopupDialog.cpp Outdated Show resolved Hide resolved
src/update/UpdateManager.cpp Outdated Show resolved Hide resolved
src/update/UpdatePopupDialog.h Show resolved Hide resolved
src/update/UpdateManager.cpp Outdated Show resolved Hide resolved
src/update/UpdateManager.h Show resolved Hide resolved
#include <wx/utils.h>

static const char* prefsUpdatePopupDialogShown = "/Update/UpdatePopupDialogShown";
static const char* prefsUpdateScheduledTime = "/Update/UpdateScheduledTime";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not very important, I would prefer to simplify with a static IntSetting object... but 32 bits may not be enough and it might be one of the 32 bit Windows builds we still do.

Maybe I should define LongSetting?

Copy link
Contributor Author

@gera-gas gera-gas Jun 17, 2021

Choose a reason for hiding this comment

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

Yes, if it possible LongLongSettings

src/update/UpdateManager.cpp Outdated Show resolved Hide resolved
ShowExceptionDialog(parent,
XO("Error checking for update"),
XO("Unable to connect to Audacity update server."),
wxTheApp->CallAfter([] {ShowExceptionDialog(nullptr,
Copy link
Collaborator

Choose a reason for hiding this comment

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

In this and the other lambdas, you use nullptr for parents of dialogs.

You might safely use FindProjectFrame(GetActiveProject()) instead. It would be sure to center the dialog on the main project window that the user is looking at.

This may be an unimportant, small advantage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I tested with wxTheApp using it was centered too. Just I want avoid from project is not created (on macOS when menu shown only) by wxTheApp using.

Clean up extra code, remake static title, move settings to UpdateManager class.
@Paul-Licameli Paul-Licameli dismissed crsib’s stale review June 18, 2021 13:46

Dmitry's suggestions have all been done

@Paul-Licameli Paul-Licameli merged commit 87d94fe into audacity:master Jun 18, 2021
Release 3.0.3 automation moved this from Review to Testing Jun 18, 2021
@Penikov Penikov moved this from Testing to Done in Release 3.0.3 Jul 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Release 3.0.3
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants