CodeRunner: 'full screen' button #578890

[danghieu1407]

Hi @timhunt , This ticket is ready for review. It involves the creation of a mustache file to incorporate both the full-screen and exit full-screen buttons. The file "fullscreen.js" manages the screen mode, ensuring that when transitioning between full-screen and exiting full-screen, the height is reset to its original value before entering full-screen mode. For the future, I have created a function getUIPluginType to know that which UI Plugin we use. And I have styling for the Fullscreen /exit full screen in style.css. Could you please review this change? Many thanks.

[timhunt]

Thanks for working on this @danghieu1407. Some comments on the code:

1. For the benefit of Richard, who has not seen a demo of this, it would be helpful to attach some screen-grabs.
2. You might like to have a read of https://moodledev.io/general/development/abc/comments. In particular, the comment "JavaScript to handle full screen." on a file called fullscreen.js reminded me of "... if essentially all the words in the comment are part of the name, ... You can do better than that!".
3. Moodle has it's own 'fullscreen' icon in core, but it is not the standard fullscreen icon most users are familiar with, and there is no matching 'exit full screen' icon, so I think we are right to supply our own.
4. However, the two icon files provided do not match - as you can see in the github preview. Should we make these icon files consistent?
5. Does doing that fix the fact that the icon is a different size in Firefox? Really, if a simple icon is coming out a different size in FF and Chrome, I feel there must be a root casue we should be fixing, rather than working around it in JavaScript.
6. What is the 'wrap-editor' div for? (it is not clear, so if it is required, a comment would be good.)
7. Since we always want the icons in the HTML, why are we rendering a template in the JavaScript, rather than just outputting them in the renderer in PHP?
8. There seems to be quite a lot of sub-optimal naming in the JavaScript.
   • FullscreenEditor is not a full-screen editor. How about EditorFullscreenToggle?
   • We are not getting the initial size of the editor, nor setting its default size. How about saveCurrentEditorSize and resetEditorToSavedSize?
   • getEditorSelector is not getting a selector, it is getting an element. How about getWrapperElement.
   • uiPlugin this is not a UI plugin, it is the type of UI plugin used, so how about uiPluginType.
9. To me, attachEvent is not a helpful function. I think it would be more helpful to have functions for what happens in response to the events (enterFullscreen and exitFullscreen) and if those are named functions, the two addEventListener calls can just be inline in the constructor.
10. To me, it seems messy to have if (getUIPluginType(uiPlugin) === 'ace') { calls everywhere. It would seem clearer to have separate classes AceEditorFullscreenToggle, and to have the only test of uiPluginType in the init method, where it then created the appropriate toggle class, which can then work without a lot of if statements.

[danghieu1407]

Hello @timhunt ,

Thank you for your feedback. Here are the updates:

1. I have added Screen-grabs to the pull request.

• Screen default mode:
• Fullscreen mode:

2. I have updated the JavaScript comment in the code.

3,4. Icon files have been made consistent.

5. Because the resize icon in Firefox is bigger than in another browser. We can't change the size of ::-webkit-resizer for Firefox, which is why I have added the code specific for Firefox. So, I removed the code specific to Firefox and adjusted the CSS for all browsers to prevent the need for extra styling.

6. The 'wrap-editor' has been removed, and I have added a comment for clarification.

7. I have changed the rendering of a template in JavaScript to PHP.

8. I have renamed:

• FullscreenEditor to EditorFullscreenToggle
• getInitialSize to saveCurrentEditorSize
• setDefaultSize to resetEditorToSavedSize
• getEditorSelector to getWrapperElement
• uiPlugin to uiPluginType

  9. enterFullscreen and exitFullscreen have been added to handle events, called in the constructor.

  10. The conditional check for 'ace' in getUIPluginType(uiPlugin) has been removed. I have created a base class EditorFullscreenToggle and also created AceEditorFullscreenToggle as a class that extends from the base class to handle specific code for different editor types.

Also, I have added z-index to address the full display of the icon border when the "tab" button is pressed. This ticket is ready for review now? Could you please review this change? Many thanks, Tim.

[timhunt]

A. Nice work with the EditorFullscreenToggle base class, and AceEditorFullscreenToggle subclass. That is a good way to organise this code.

This is now close, but I think there are still some things to fix:

B. You misinterpreted what I wrote about enterFullscreen. In your code, the enterFullscreen function is acutally doing "add event listener to ..." which is very confusing. I was expecting the code in the initFullScreenToggle function to be this.fullScreenButton.addEventListener('click', this.enterFullscreen); (you might need to add a .bind(this) there???) then, enterFullscreen is a function that takes argument e and is actually a funcition that makes the editor full-screen.

C. As far as I can see, in the init() method, the variable 'editorObject' is never used. Therefore, I think it can be removed.

D. In the renderer, you should not refer to global $OUTPUT. Codechecker will tell you this. It should be $this->render_from_template('qtype_coderunner/screenmode_button', []);. $options->context is wrong.

E. if( with no space will also fail CodeChecker. (Note the Moodle plugin CI config here means this is not failing the CI build, but you should still check the results.)

[danghieu1407]

Hi @timhunt, Thanks for your review. B. I have understand now. I have added this.fullScreenButton.addEventListener('click', this.enterFullscreen.bind(this, wrapperEditor)) and this.exitFullscreenButton.addEventListener('click', this.exitFullscreen.bind(this)) to initFullScreenToggle. C. I have removed a variable ''editorObject'. And call initFullScreenToggle function inline. D. I have removed $OUTPUT and changed that to $this->render_from_template('qtype_coderunner/screenmode_button', []). E. I have updated the if and checked the CI result. Could you please review this change? Many thanks.

[timhunt]

That looks good now. I think you should rebase this to squash it down to 1 commit, then it is ready for Richard to give a final review.

[danghieu1407]

Hi @timhunt, I have squashed to a single commit. Thanks for your review Tim

[timhunt]

@trampgeek, this should now be ready for your review.

[trampgeek]

Thanks for all the work on this, guys. I downloaded it into my development branch and it works nicely for Ace UIs. I like the discreet icons you've chosen.

I do have some concerns that I'd like to discuss, however. It currently works only for the Ace UI, and only within the the student's view of a question. It doesn't work for a question author (e.g. the question template, sample answer etc) and it doesn't work for an Ace editor within the Scratchpad UI (more on this later). And at present it doesn't work with any other UIs except the Ace gapfiller UI.

Now I do appreciate that other UI plugins could add their own subclass of EditorFullscreenToggle to the fullscreen.js file and I was easily able to make the GraphUI go full screen too. I simply used a clone of the AceEditorFullScreenToggle class without the four lines that directly save and reset the height and width of the wrapper. But my feeling is that it shouldn't be necessary to tweak the CSS settings of the content of a wrapper. The InterfaceWrapper class should manage the sizing of the UI plugin it's managing by calling the UI plugin's resize method. If the plugin has any special requirements when being resized, it can implement them within that resize method.

Interestingly, those four lines involved in explicitly managing the size of the .ace_editor and .ace_content are the only Ace-specific code within your implementation, except for the init method that restricts the functionality to the Ace UI (and Ace gapfiller UI). I'm not even sure why those lines are there. I did a quick test without them and everything seemed to work, but I haven't looked closely.

The Scratchpad UI is an interesting special case. It's new but the little feedback I've received suggests it might become the standard UI for question authors, rather than the Ace UI. Have you guys tried it? It has two Ace windows inside it, one for the student answer and another for testing, that allows students to develop code without submitting it for grading. Students love it. Too much, in fact - we struggle to wean them off it into an IDE!

The Scratchpad UI is a tricky case for resizing as it has a user interface wrapper that wraps the whole UI, but within it there are two Ace editors, each with its own user interface wrapper. This is a case where you wouldn't want the full screen mode to operate on the embedded Ace editors but only on the whole UI. Again it seems to me that resizing should be managed via the resize method of the UI plugin, not by the outside world explicitly setting CSS values.

In short, while I like the simply functionality you've provided I'm a bit unhappy about its generality. Did you guys consider providing the functionality via the InterfaceWrapper class rather than by CSS tweaks?

[danghieu1407]

Hi @trampgeek, Thank you for your review and feedback. I plan to make the following enhancements to the code:

1. I will enable fullscreen not only for students but also for authors.
2. The fullscreen and exit fullscreen features will apply to all editors in the question type "coderunner."
3. I make the function saveCurrentEditorSize and resetEditorToSavedSize because when a user elongates the editor and enters fullscreen mode, I want to save that height. Upon exiting fullscreen, the height will revert to its previous size. To achieve this, I'll incorporate the logic into the resize function. Additionally, I'll move the fullscreen logic to InterfaceWrapper.

4. For the scratchpad UI, I intend to add a fullscreen button at the bottom right corner of the editor, as illustrated in the image below:

   While in fullscreen mode, the exit fullscreen button will occupy the same position as the fullscreen button, as depicted in the image below:

Is this aligned with your suggestion? I would like to hear your thoughts on these proposed changes. Many thanks

[trampgeek]

Many thanks for the proposed changes. I think moving the code into the InterfaceWrapper class is definitely the right way to go. If you do that, you should be able to revert your changes to renderer.php and the functionality will be available to question authors as well as students.

The case of the Scratchpad UI is an interesting one. I think the decision as to how to use the screen real estate in full screen mode should be within the Scratchpad UI itself, rather than treating it as a special case at a higher level. For example, the UI might prefer to allocate a much larger percentage of the screen to the main window rather than the scratchpad, or vice versa. To make this feasible, I suggest the following, although this is just my first thoughts so please feel free to suggest alternatives.

1. We could specify that the json for each UI plugin can take an optional allowFullScreen parameter. The default value, in the absence of this parameter, would be false. In the first instance, if the goal is to allow full screening of just the ace-ui and ace-gapfiller-ui, only the .json files for those two plugins would supply this value, setting it to true.
2. The newUiWrapper function (in userinterfacewrapper.php) and the associated InterfaceWrapper constructor could take an optional third parameter, also called allowFullScreen, which defaults to null. The only context in which the function would ever be called with this parameter explicitly supplied would, currently, be within scratchpad UI. Its purpose is to override the value supplied by the .json file for the Ace editor in order to disable full screen mode for the embedded Ace editors. It might potentially be used in other complex UIs, however.

Then, within the InterfaceWrapper class, the value of allowFullScreen is taken from the function call parameter if non-null or from the UI parameter set (in the data-params attribute of the element) if supplied, defaulting to false.

I don't expect you to take on the task of defining the full screening behaviour of the scratchpad ui, though you're welcome to do so if you wish. The only change required to preserve the existing behaviour is to add the third parameter false to the two calls to newUiWrapper in addAceUis.

How does that sound to you? As I say, it's just my provisional thoughts. I'll also bounce the thoughts off the guy in our department who wrote the scratchpad UI to see if he agrees.

Many thanks for agreeing to continue working on this. I think you're on the right track!

[danghieu1407]

Thank you, @trampgeek. I'll proceed with this solution. Regarding the Scratchpad UI, if he approves, I'll proceed with the implementation.

[danghieu1407]

Hi @trampgeek ,

I've created a new option in the JSON file based on your suggestion. However, when logging the data using params.<name of option> in question/type/coderunner/amd/src/userinterfacewrapper.js, it remains undefined. Upon investigation, I found that the function updated_params in question/type/coderunner/classes/ui_parameters.php consistently returns empty due to $param->updated being set to false.

This false setting occurs because the function evaluate_merged_ui_parameters in question/type/coderunner/question.php calls a new instance of qtype_coderunner_ui_parameters, where $param->updated is always false. I'm unsure if there is an issue in the updated_params function or if I've missed a configuration step. Could you please review this and provide guidance? Thank you.

[trampgeek]

Yes, you're quite right of course. It seems that only those UI parameters that have been changed from their default values are recorded in the data-params attribute. This is clearly a deliberate policy, the assumptions being that the parameters are for use only by the UI plugin itself, not the wrapper, and that the plugin knows its own default values.

My apologies for sending you down a bad route.

I've been mulling over alternative strategies. I first thought it would be easiest to change the existing design so that all ui parameters are recorded in the data-params, making them available to the wrapper. But I'm having doubts about that. The ui parameters editing panel uses the json file for the current UI to present the available options to the user. For consistency, all json files would need to have an allowFullScreen value defined, which adds UI clutter, and I can't see this as being a useful UI feature anyway. Furthermore, I rather like the idea of minimising the unnecessary UI parameters in the HTML.

I've just been discussing the problem of the Scratchpad UI with its author, James Napier. He points out that he has suppressed the resize of the whole UI wrapper, so there's nowhere to put the full-screen icon anyway.

We have experimented a bit and we think that full-screening either of the individual Ace editor wrappers seems to work fine.

My new proposal, then, is that we add an optional allowFullScreen() method to the UI plugin API, which can be called by the user interface wrapper constructor to determine whether or not to add the full screen icon to the resize bar at the bottom of the newly created wrapper. If a plugin does not have this method, assume true. At present, the only UI I can think of that will require an explicit implementation is the scratchpad UI, which will return false.

Something like the following test can be done in the userinterface wrapper to check for full screening:

let canDoFullScreen = uiInstance.allowFullScreen?.() ?? false;

Moodle's grunt calls rollup, which should transpile the ?. and ?? operators but please check this. Expand the code if necessary.

The event handler for the full-screen icon can do pretty much what it currently does, but should also call the plugin's resize() method after going in or out of full screen mode so that the plugin can take action (e.g. scaling content).

[danghieu1407]

Hello @trampgeek, I'm currently developing your updated solution. I'm unsure why this Pull Request has been merged. Should it have merged, I plan to submit a new Pull Request to improve this code. Is that ok with you? Many thanks

[trampgeek]

@danghieu1407 Sure, please do keep developing and submit a pull request. I intended to do a temporary merge to check out what you'd done but it somehow managed to propogate back to github. Since then, though, I've removed your code and should be in a position to accept a new pull request when you're ready.

Thanks for keeping on working on this.

[danghieu1407]

Hi @trampgeek, I have created a new pull request for fixing based on your suggestion. Pull Request Could you please review this change? Many thanks.

[trampgeek]

Sorry about the delay danghieu. I'm a bit swamped with work at the moment, but will try to get onto it by the end of the coming weekend, at least.

[danghieu1407]

Thank you so much @trampgeek.