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.

Sign up for GitHub

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

Jump to bottom

Tours system improvements #26454

Merged
merged 9 commits into from
Feb 19, 2025
Merged

Tours system improvements #26454

merged 9 commits into from
Feb 19, 2025

Conversation

Eism
Copy link
Contributor

@Eism Eism commented Feb 12, 2025 •
edited
Loading

Resolves: #26342
Resolves: #26341
Resolves: #26300

@Eism Eism requested a review from RomanPudashkin February 12, 2025 09:36
@Eism Eism force-pushed the tours_stabilization branch 2 times, most recently from 7521434 to e3bc132 Compare February 12, 2025 09:38
@Eism Eism requested a review from igorkorsukov February 12, 2025 11:35
@Eism Eism changed the title fixed #26342: Tours popup: match visual style to design spec Tours popup. Fixed design Feb 12, 2025
@Eism Eism changed the title Tours popup. Fixed design Tours system improvements Feb 12, 2025
@avvvvve
Copy link

avvvvve commented Feb 12, 2025 •
edited
Loading

@Eism The dismissal behavior is looking good for the most part! A couple small things:

  • When the tour popup is open, opening a dialog (i.e. Styles, Preferences, Parts) will dismiss it. Is there any way we can make it stay open while dialogs are open? (If it's difficult to accomplish, doing this should not block us from merging this PR)

  • The padding is still too small around the tours popup content; it should be 12px as shown in Figma:

    image

@Eism Eism force-pushed the tours_stabilization branch from 8e7b01d to 6f15e30 Compare February 12, 2025 14:05
@avvvvve
Copy link

avvvvve commented Feb 12, 2025

Thanks Elnur!

Approving this with the caveat that the very first time I opened this build, the tour popup appeared before the Nucleus dialog and the tour got dismissed (as was previously happening). I haven't been able to reproduce it after resetting preferences. Maybe it had something to do with assets in the Nucleus dialog taking a second to load, and now they're cached?

@Eism Eism force-pushed the tours_stabilization branch from 6f15e30 to ea46b4f Compare February 17, 2025 09:40
@bkunda
Copy link

bkunda commented Feb 18, 2025 •
edited
Loading

Tested on macOS. Can confirm:

  • The order and timing of notifications is now correct ✅
  • Tours popup can be dismissed only by the user when clicking one of its buttons ✅

I did note the tour is dismissed if you navigate to a different tab (e.g. "Home" and then back to "Score"), however I don't think this is a problem.

Happy to approve from my end. @DmitryArefiev let us know if you uncover anything else noteworthy 🙂

@Eism Eism force-pushed the tours_stabilization branch 2 times, most recently from 58b1d03 to f3d36dd Compare February 18, 2025 11:07
@bkunda
Copy link

bkunda commented Feb 18, 2025

Retested on win and macOS. Much improved.
Thanks @Eism!

Eism added 8 commits February 18, 2025 18:04
@Eism
fixed musescore#26342: fixed ui
a3b6560
@Eism
show tours right after muse sounds promo dialog
b1f7e1a
@Eism
fixed margins and anchorItem for tours
334d2de
@Eism
wait for the program to activate to show the tour
7230425 
before the tour is shown, a promo dialog is shown and if the user goes to the browser to see the update information, then the tour will close immediately since the program does not active
@Eism
fixed showing tours and promo dialog when restoring the last session
07f1da1
@Eism
disabled auto close for tours (win+macos)
a123949 
if user clicked on control we will close tour step and show next one
if tour's step opened, the control's tooltip will not be shown
@Eism
no need to request update again: if there is no update - skip
d4951c7
@Eism
added translation for tours
77fb2ce
@Eism Eism force-pushed the tours_stabilization branch from 7ac704f to 77fb2ce Compare February 18, 2025 16:04
RomanPudashkin
RomanPudashkin reviewed Feb 19, 2025
View reviewed changes
src/framework/tours/internal/toursservice.cpp
step.videoExplanationUrl = itemObj.value("video_explanation_url").toString();
step.controlUri = Uri(itemObj.value("control_uri").toString());

JsonObject itemLocaleObj = itemObj.value("locale").toObject();

if (!itemLocaleObj.contains(locale)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need this code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will keep this type of registration for the future

RomanPudashkin
RomanPudashkin reviewed Feb 19, 2025
View reviewed changes
src/framework/tours/view/toursprovider.cpp
}
}

bool ToursProvider::canControlTourPopupClosing() const
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe just autoCloseTourPopup()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

canControlTourPopupClosing looks more like bool method

@Eism Eism force-pushed the tours_stabilization branch from f8b13b9 to 853e826 Compare February 19, 2025 10:17
@Eism Eism force-pushed the tours_stabilization branch from 853e826 to bdcd446 Compare February 19, 2025 10:17
@RomanPudashkin RomanPudashkin merged commit 238aa97 into musescore:master Feb 19, 2025
11 checks passed
@RomanPudashkin RomanPudashkin deleted the tours_stabilization branch February 19, 2025 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@igorkorsukov igorkorsukov igorkorsukov approved these changes

@RomanPudashkin RomanPudashkin RomanPudashkin approved these changes

Assignees

@avvvvve avvvvve

@DmitryArefiev DmitryArefiev

Labels
None yet
Projects
None yet
Milestone
No milestone
6 participants
@Eism @avvvvve @bkunda @igorkorsukov @RomanPudashkin @DmitryArefiev