Update the MusicKeyboard widget on maximizing. #2857

daksh4469 · 2021-02-24T10:56:11Z

Issue Reference: #2767, #2808
This PR improves the UI and fixes some bugs in the music keyboard widget.

Firstly, there is an unnecessary scroll bar present (as shown below) on the right-most side of the widget which can be removed without any issues.

Secondly, the main issue was on maximizing this widget, as on maximizing it there was plenty of space left which was unused but could be used for a better User Experience. This PR enables this widget to enlarge the note matrix on maximizing, so as to take full advantage of a full screen.

Before:

mkscreen-capture.1.mp4

After: (without the right-most scroll bar as mentioned above)

mkscreen-capture.2.mp4

daksh4469 · 2021-02-24T10:58:44Z

Please Review this @meganindya @walterbender.
Thanks.

walterbender · 2021-02-24T14:38:20Z

I need to do a bit more testing, but I am seeing the note duration cut off at the bottom of the screen.
Also, there seems to be a regression -- not introduced in this patch -- that prevents one from clicking on the lower 2/3 of the grid.

js/widgets/status.js

Co-authored-by: Vaibhav D. Aren <[email protected]>

daksh4469 · 2021-02-25T07:35:48Z

I am seeing the note duration cut off at the bottom of the screen.

Also, there seems to be a regression -- not introduced in this patch -- that prevents one from clicking on the lower 2/3 of the grid.

I will try to resolve these issues. @walterbender

daksh4469 · 2021-02-25T12:14:49Z

I am seeing the note duration cut off at the bottom of the screen.

I used the white space below it so as to eliminate the need for a vertical scroll.
Would this work? @walterbender

Also, I noticed another regression (not introduced in this patch), in which, if multiple notes are selected to be played at one time, then on clicking the Play button, the sound will stop at that particular step and not go any further.

screen-capture.mkr.mp4

…blocks into daksh4469-master

walterbender · 2021-02-25T12:45:20Z

Yes. The duration does not seem to be getting cut off.

One other weirdness I see: When you go to full screen, the grid does not expand until you play another note.

daksh4469 · 2021-02-25T13:29:43Z

One other weirdness I see: When you go to full screen, the grid does not expand until you play another note.

I rechecked this, and this doesn't seem to be happening in my system. Can you please verify this? @walterbender

MusicKeyboard.mp4

walterbender · 2021-02-25T13:37:47Z

I was in Chromium on Fedora 33. I'll check the same with Firefox.

daksh4469 · 2021-02-25T13:50:21Z

I also checked this firefox and this was working fine there too.

Firefox_MuiscKeyboard.mp4

ricknjacky

Maximizing doesn't work as you have described :-

Lot of "unused" space at the bottom, just like there was previously

OS: Windows
Browser: Brave(chromium based)

js/widgets/musickeyboard.js

daksh4469 · 2021-02-25T14:07:16Z

OS: Windows
Browser: Brave(chromium based)

Ohh....I have checked this on Google Chrome and Mozilla Firefox.
There, it worked perfectly.

ricknjacky · 2021-02-25T14:08:51Z

OS: Windows
Browser: Brave(chromium based)

Ohh....I have checked this on Google Chrome and Mozilla Firefox.
There, it worked perfectly.

;-; Brave is chromium based, more of it is exactly google chrome tbh.

daksh4469 · 2021-02-25T14:26:52Z

I now also checked on Brave browser and got the same results:

Brave.mp4

walterbender · 2021-02-26T00:40:12Z

Version 88.0.4324.150 (Developer Build) Fedora Project (64-bit)
Also on FF, 85.0.1 (64-bit)

daksh4469 · 2021-02-26T05:29:33Z

So, is there any error shown in the console related to this or it is just not working? @walterbender @ricknjacky
Also, can you suggest how should I test this as on my system it works fine on all browsers?

daksh4469 · 2021-02-26T07:39:23Z

Regarding the issue where the cells are "unclickable" in the bottom around 2/3rd part of the grid (bottom 7 to be precise), it is happening as the for loop (starting from line 1057 in the makeClickable function), the parameter this.layout.length is getting set to 5, after #2849, instead of 12.
Now, the error arises in the _keysLayout function where the notes the are pushed in the layout array. In #2849, I think there is a mistake in lines 953 through 960 wherein, after #2849, is:

for (let i = 0; i < sortedList.length; i++) {
            this.layout.push({
                noteName: sortedList[i].noteName,
                noteOctave: sortedList[i].noteOctave,
                blockNumber: sortedList[i].blockNumber,
                voice: sortedList[i].voice
            });
}

In this code, sortedList should be replaced by newList (newList = fillChromaticGaps(sortedNotesList)). Here, the sortedList.length = 5 which is correct but it makes the for loop iterate only 5 times and thus limiting the layout array to a length of 5. Thus, in the for loop, it should be i < newList.length.
I did this and the widget has been working as expected without the above-mentioned issue.
This is also done in the current release of musicblocks.
Please check this @walterbender.
If you approve this, I would commit these changes.

ricknjacky · 2021-02-26T09:53:45Z

js/widgets/status.js

-                cell.style.fontSize = Math.floor(this._cellScale * StatusMatrix.FONTSCALEFACTOR)*0.90 + "%";
-                if(!(this.isMaximized)){
+                cell.style.fontSize =
+                    Math.floor(this._cellScale * StatusMatrix.FONTSCALEFACTOR) * 0.9 + "%";


Status is different widget afaik, why is this file a part of this PR?

Sorry, I missed this file previously. But please do tell me If I have missed something and what has this file to do with maximizing musickeyboard widget in particular.

Yeah, status is a different widget. Actually, after #2838 was merged, as always I deleted that branch and then realized that I left debug statements (that I used for #2838) in the code and did not Prettify the code. So just to rectify that, I did not feel that a different PR would be needed. Thus, I committed those changes first, so this could be rectified.

ricknjacky · 2021-02-26T09:54:48Z

js/widgets/status.js

@@ -120,7 +120,8 @@ class StatusMatrix {
            const row = header.insertRow();

            cell = row.insertCell(); // i + 1);
-            cell.style.fontSize = Math.floor(this._cellScale * StatusMatrix.FONTSCALEFACTOR)*0.90 + "%";
+            cell.style.fontSize =
+                Math.floor(this._cellScale * StatusMatrix.FONTSCALEFACTOR) * 0.9 + "%";


can you please explain what you've done here?

As I mentioned in your previous review, this is just prettifying the code (as mentioned in #2609) and removing debug statement.

ricknjacky · 2021-02-26T09:55:03Z

js/widgets/status.js

@@ -176,7 +177,8 @@ class StatusMatrix {
        if (_THIS_IS_MUSIC_BLOCKS_) {
            const row = header.insertRow();
            cell = row.insertCell();
-            cell.style.fontSize = Math.floor(this._cellScale * StatusMatrix.FONTSCALEFACTOR)*0.90 + "%";
+            cell.style.fontSize =
+                Math.floor(this._cellScale * StatusMatrix.FONTSCALEFACTOR) * 0.9 + "%";


^^same here

ricknjacky

So, is there any error shown in the console related to this or it is just not working? @walterbender @ricknjacky
Also, can you suggest how should I test this as on my system it works fine on all browsers?

Not sure what you mean by working fine, I tested again and

there is still lot of unused space at the bottom
maximize funcn doesn't work as it should, instead it malfunctions

Bug.mp4

See console log on the right of the screen

daksh4469 · 2021-02-26T12:15:20Z

So, is there any error shown in the console related to this or it is just not working? @walterbender @ricknjacky
Also, can you suggest how should I test this as on my system it works fine on all browsers?

Not sure what you mean by working fine, I tested again and

there is still lot of unused space at the bottom

maximize funcn doesn't work as it should, instead it malfunctions

Thanks for the feedback.
I am working on the case where there is no grid. Also, I think using relative lengths instead of static lengths would fix this.
Because as the developer tools tabs get larger, the cell-height diminishes and thus unused space increases (due to static lengths). Also, if in full-screen (without the developer tools tabs), is the unused space (white space) minimal?

…o daksh4469-master

walterbender · 2021-03-12T12:25:10Z

I am still seeing the same problem (tested on Chromium). I play some notes;

Then click to expand the window:

Not until I play more notes does the grid display properly.

daksh4469 · 2021-03-12T13:01:14Z

I am trying to figure out this. Don't know why it is working on my device. @walterbender

daksh4469 · 2021-03-12T14:25:56Z

Since it is not maximized until a new note is added, I suspect this could be happening in the widgetWindow.onmaximize event. Thus, I changed it to this.widgetWindow.onmaximize.
Could you check this, please? @walterbender

walterbender · 2021-03-12T14:46:59Z

I've just tested on a small screen and as soon as I click on any note, it jumps to full screen. Was that the intention?

daksh4469 · 2021-03-12T14:52:59Z

Is it just maximizing on clicking on the keyboard? If yes, then no actually. This should not be happening.

walterbender · 2021-03-12T14:54:30Z

Yes... just clicking on the keyboard.

walterbender · 2021-03-12T14:55:07Z

Although it seems to happen even w/o your latest changes.

daksh4469 · 2021-03-12T16:06:08Z

The state of widgetWindow_maximized is got through widgetWindows.js file and there are no changes done there, and, its state is not affected in this musickeyboard.js file also. Also, what I have done is set the styles according to the condition of whether the widget is maximized or not. This condition is applied in 4 cases:

When the widget is maximized, i.e., the onmaximize event.
When the erase button is clicked.
When a note is added through a click event. (__endNote method taking element as an argument)
When a note is added through keyboard input event. (__endNote method taking event as an argument)

Now, I can't figure out why this won't work.

walterbender · 2021-03-12T23:57:36Z

I just realized that clicking on the "full screen" button vs double-clicking on the toolbar exhibits different behavior. Double-clicking is where I experience the problem. Clicking the button works fine. The double-click handler was missing some steps. Works fine now.

walterbender · 2021-03-12T23:59:22Z

a795c8e

daksh4469 · 2021-03-13T04:42:07Z

Thanks @walterbender :)

daksh4469 added 3 commits February 24, 2021 15:39

Refactor status.js

8d61c5d

Feature: Maximizing action on musickeyboard

8c0b1bf

Refactored some code

2b9f751

vaibhavdaren reviewed Feb 25, 2021

View reviewed changes

js/widgets/status.js Outdated Show resolved Hide resolved

Update js/widgets/status.js

2f4eccc

Co-authored-by: Vaibhav D. Aren <[email protected]>

daksh4469 added 2 commits February 25, 2021 18:01

Eliminate vertical scroll to fix issue

cba6800

Merge branch 'daksh4469-master' of https://github.com/daksh4469/music…

80f4a44

…blocks into daksh4469-master

ricknjacky suggested changes Feb 25, 2021

View reviewed changes

ricknjacky reviewed Feb 25, 2021

View reviewed changes

js/widgets/musickeyboard.js Show resolved Hide resolved

ricknjacky reviewed Feb 25, 2021

View reviewed changes

js/widgets/musickeyboard.js Show resolved Hide resolved

Handle Overflows and set scroll

216b19d

ricknjacky reviewed Feb 26, 2021

View reviewed changes

ricknjacky suggested changes Feb 26, 2021

View reviewed changes

daksh4469 and others added 4 commits February 26, 2021 18:03

Fix issue in case of no grid

4a3f131

docs: target more beginners

4b73c9e

docs: add link for MB 2.0

2e40ecd

Merge branch 'master' of https://github.com/sugarlabs/musicblocks int…

0b14fd4

…o daksh4469-master

walterbender mentioned this pull request Mar 12, 2021

Fix regressions in MusicKeyboard widget #2863

Merged

Check maximizing action

7aa8aa5

walterbender merged commit 0f906f7 into sugarlabs:master Mar 12, 2021

daksh4469 deleted the daksh4469-master branch March 13, 2021 04:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update the MusicKeyboard widget on maximizing. #2857

Update the MusicKeyboard widget on maximizing. #2857

daksh4469 commented Feb 24, 2021

daksh4469 commented Feb 24, 2021

walterbender commented Feb 24, 2021

daksh4469 commented Feb 25, 2021

daksh4469 commented Feb 25, 2021 •

edited

Loading

walterbender commented Feb 25, 2021 •

edited

Loading

daksh4469 commented Feb 25, 2021

walterbender commented Feb 25, 2021

daksh4469 commented Feb 25, 2021

ricknjacky left a comment •

edited

Loading

daksh4469 commented Feb 25, 2021

ricknjacky commented Feb 25, 2021

daksh4469 commented Feb 25, 2021

walterbender commented Feb 26, 2021

daksh4469 commented Feb 26, 2021 •

edited

Loading

daksh4469 commented Feb 26, 2021 •

edited

Loading

ricknjacky Feb 26, 2021

daksh4469 Feb 26, 2021

ricknjacky Feb 26, 2021

daksh4469 Feb 26, 2021

ricknjacky Feb 26, 2021

ricknjacky left a comment

daksh4469 commented Feb 26, 2021

walterbender commented Mar 12, 2021 •

edited

Loading

daksh4469 commented Mar 12, 2021

daksh4469 commented Mar 12, 2021

walterbender commented Mar 12, 2021

daksh4469 commented Mar 12, 2021

walterbender commented Mar 12, 2021

walterbender commented Mar 12, 2021

daksh4469 commented Mar 12, 2021

walterbender commented Mar 12, 2021

walterbender commented Mar 12, 2021

daksh4469 commented Mar 13, 2021

Update the MusicKeyboard widget on maximizing. #2857

Update the MusicKeyboard widget on maximizing. #2857

Conversation

daksh4469 commented Feb 24, 2021

daksh4469 commented Feb 24, 2021

walterbender commented Feb 24, 2021

daksh4469 commented Feb 25, 2021

daksh4469 commented Feb 25, 2021 • edited Loading

walterbender commented Feb 25, 2021 • edited Loading

daksh4469 commented Feb 25, 2021

walterbender commented Feb 25, 2021

daksh4469 commented Feb 25, 2021

ricknjacky left a comment • edited Loading

Choose a reason for hiding this comment

daksh4469 commented Feb 25, 2021

ricknjacky commented Feb 25, 2021

daksh4469 commented Feb 25, 2021

walterbender commented Feb 26, 2021

daksh4469 commented Feb 26, 2021 • edited Loading

daksh4469 commented Feb 26, 2021 • edited Loading

ricknjacky Feb 26, 2021

Choose a reason for hiding this comment

daksh4469 Feb 26, 2021

Choose a reason for hiding this comment

ricknjacky Feb 26, 2021

Choose a reason for hiding this comment

daksh4469 Feb 26, 2021

Choose a reason for hiding this comment

ricknjacky Feb 26, 2021

Choose a reason for hiding this comment

ricknjacky left a comment

Choose a reason for hiding this comment

daksh4469 commented Feb 26, 2021

walterbender commented Mar 12, 2021 • edited Loading

daksh4469 commented Mar 12, 2021

daksh4469 commented Mar 12, 2021

walterbender commented Mar 12, 2021

daksh4469 commented Mar 12, 2021

walterbender commented Mar 12, 2021

walterbender commented Mar 12, 2021

daksh4469 commented Mar 12, 2021

walterbender commented Mar 12, 2021

walterbender commented Mar 12, 2021

daksh4469 commented Mar 13, 2021

daksh4469 commented Feb 25, 2021 •

edited

Loading

walterbender commented Feb 25, 2021 •

edited

Loading

ricknjacky left a comment •

edited

Loading

daksh4469 commented Feb 26, 2021 •

edited

Loading

daksh4469 commented Feb 26, 2021 •

edited

Loading

walterbender commented Mar 12, 2021 •

edited

Loading