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

Implementing scrolling on the phrase maker #3311

Merged
merged 9 commits into from
Aug 29, 2023

Conversation

MohitGupta14
Copy link
Contributor

@MohitGupta14 MohitGupta14 commented Jul 14, 2023

@pikurasa
Copy link
Collaborator

For issue #1144 Uploading WidgetWindowScrolling.webm…

Is this suppose to be a video? Please try to reupload.

@pikurasa
Copy link
Collaborator

I tested for both vertical and horizontal "overpopulation" of cells, and I don't see a way to view the cells that are off the screen.

Screenshot at 2023-07-16 20-38-13
Phrase Maker

@MohitGupta14
Copy link
Contributor Author

@pikurasa i uploaded the video again .

@pikurasa
Copy link
Collaborator

I tried again. I am simply not getting the same results as you even after expanding the phrase maker window.

...actually, "aha! I see the difference between what you are doing and what I am doing."

There is a couple different ways of adding stuff to the widget.

The way that this PR is not accounting for is this: The user can program the widget by adding or removing blocks and initiate the widget.

My tests were as follows:

  • Pull out G major or "C major Scale" phrase maker macros; then multiply "mode length" by 3 or 4. This will make the Phrase Maker long vertically
  • Pull out any Phrase Maker, multiply the rhythm by 8-16. This will make the Phrase Maker wide horizontally.

This patch does not yet account for either of those methods.

@pikurasa
Copy link
Collaborator

pikurasa commented Aug 6, 2023

This is a big improvement.

That being said, I do think there are some areas for improvement.

  • For pitch staircase and keyboard, it seems that those have a vertical scrollbar by default and they have two vertical scrolls when you add more into the vertical space. We only need one of these, and we don't need a scrollbar by default unless the space is overflowing.
    why-two-pitch-staircase
    two-scrolls-keyboard-widget

  • When there is overflow, it would be nice if both the bottom vertical scroll and horizontal were always visible/usable. When there is both vertical and horizontal overflow, the user needs to scroll one to see the other. For example:
    top
    bottom

  • When you click to expand the screen, it is only filling 70% of the space. This is a bit of a regression in this patch as it fills the space in the master branch.
    Screenshot at 2023-08-05 21-55-28
    (After expanding:)
    Screenshot at 2023-08-05 21-54-42

  • It would be nice if the top (and left, if it is there) menu were always visible. Right now, if you scroll down vertically, you can no longer see the top.
    Screenshot at 2023-08-05 21-55-12

These are the four points of feedback that I think would improve what you implemented further.

@pikurasa
Copy link
Collaborator

What is improved:

  • For the phrase maker, we can always see the scroll bars when we need to.
  • Phrase maker window size automatically resizes when it is too big.

What needs more work:

  • The top menu for phrase maker is not visible. This is a regression.
  • Rhythm maker does not resize, and it's vertical scrollbar never appears
  • For Music Keyboard, the pitch names to the left should always be visible. Right now, they are not when a user chooses a large space.
  • The pitch staircase does not have a way to go to fullscreen. (This is how it has always been, and is perhaps not too important.) FWIW, the top menu of pitch staircase works very well in terms of always being visible, so is a good example in that regard.

New test file:

@MohitGupta14
Copy link
Contributor Author

What is improved:

  • For the phrase maker, we can always see the scroll bars when we need to.
  • Phrase maker window size automatically resizes when it is too big.

What needs more work:

  • The top menu for phrase maker is not visible. This is a regression.
  • Rhythm maker does not resize, and it's vertical scrollbar never appears
  • For Music Keyboard, the pitch names to the left should always be visible. Right now, they are not when a user chooses a large space.
  • The pitch staircase does not have a way to go to fullscreen. (This is how it has always been, and is perhaps not too important.) FWIW, the top menu of pitch staircase works very well in terms of always being visible, so is a good example in that regard.

New test file:

these changes are just for phrase maker .

@pikurasa
Copy link
Collaborator

these changes are just for phrase maker .

I tested it, and it works well.

@MohitGupta14 Question: How tricky is it to also have the row at the bottom that has "note value" always visible at the bottom?

Screenshot at 2023-08-20 20-41-43

If it is tricky, I think we can just leave it here and have Walter review the code. If it's not too tricky, it would be a big improvement.

The reason it is nice to have always showing is because it has important information about the note length, and is also an important place where the user can manipulate note value if they want.

@MohitGupta14
Copy link
Contributor Author

these changes are just for phrase maker .

I tested it, and it works well.

@MohitGupta14 Question: How tricky is it to also have the row at the bottom that has "note value" always visible at the bottom?

Screenshot at 2023-08-20 20-41-43

If it is tricky, I think we can just leave it here and have Walter review the code. If it's not too tricky, it would be a big improvement.

The reason it is nice to have always showing is because it has important information about the note length, and is also an important place where the user can manipulate note value if they want.

please review the latest commit. it makes the bars sticky for both keyboard and as well phrase maker

@pikurasa
Copy link
Collaborator

@MohitGupta14 The one remaining issue for this is that the bottom row of Music Keyboard is not visible when the widget is expanded. Have you figured out the cause of that yet?

Otherwise, the results of this are good.

@pikurasa
Copy link
Collaborator

@walterbender This is ready for your review.

@pikurasa pikurasa requested a review from walterbender August 25, 2023 12:22
@walterbender walterbender merged commit 03b419c into sugarlabs:master Aug 29, 2023
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.

3 participants