Saving playground code state #1917

mani-chand · 2024-03-11T17:15:06Z

The embedded Playground's code reset when you navigate between slides. This has caused problems: if people navigate away from a slide to look something up, they've suddenly lost their work.

djmitche · 2024-03-11T19:44:51Z

from @fw-immunant in #1476 (comment)

It seems like the text editor widget (ace_text) should have some API for getting/setting text contents, rather than digging around in its internal markup.

From the docs, it looks like these suffice: editors[0].getValue();/editors[0].setValue("fn main(){\n\n}", -1);.

While teaching I rely on being able to reset the code samples to their original contents (both while discussing a slide and before teaching the course a second time), so I'd like to make sure there's still a straightforward way to do so before this lands.

fw-immunant

See individual comments.

I think for usability reasons this feature also requires that there's a way to reset the code on an individual page to its original state, the way reloading the page did previously. In addition, after teaching the full course, instructors need a way to "erase the blackboards" and reset all code samples to their pristine state.

fw-immunant · 2024-03-11T20:47:53Z

theme/redbox.js

+
+function setCodeToPlayground() {
+  const code = JSON.parse(localStorage.getItem(window.location.href));
+  console.log(code);


Remove debugging printing.

fw-immunant · 2024-03-11T20:49:29Z

theme/redbox.js

+    const playground = document.getElementsByClassName("ace_text-layer")[0];
+    while (playground.firstElementChild) {
+      console.log(playground.firstElementChild.innerText.replace(/\s+/g, " "));
+      playground.removeChild(playground.firstElementChild);
+    }
+    console.log(playground, "after removal");
+    for (let i = 0; i < code.length; i++) {
+      let parentDiv = code[i][0];
+      let spanChild = code[i][1];
+      let div = document.createElement(parentDiv.tag);
+      div.style.height = "17.5938px";
+      div.style.top = `${17.5938 * i}px`;
+      for (let cls in parentDiv.classes) {
+        div.classList.add(parentDiv.classes[cls]);
+      }
+      for (let j = 0; j < spanChild.length; j++) {
+        //console.log(spanChild[j].styles,typeof(spanChild[j].styles))
+        let span = document.createElement(spanChild[j].tag);
+        //span.classList = spanChild[j].classes
+        for (let cls in spanChild[j].classes) {
+          span.classList.add(spanChild[j].classes[cls]);
+        }
+        span.innerText = spanChild[j].text;
+        div.insertBefore(span, div.lastChild);
+      }
+      playground.insertBefore(div, playground.lastChild);
+    }
+  }


Use the API of the editor component rather than poking into its portion of the DOM:

editors[0].setValue("fn main(){\n\n}", -1);

or similar.

fw-immunant · 2024-03-11T20:52:07Z

theme/redbox.js

+  localStorage.removeItem(window.location.href);
+}
+
+window.onunload = setTimeout(setCodeToPlayground, 5000);


I believe this is a type error. Isn't window.onunload supposed to be a callable? setTimeout returns an integer timeout id.

What is this intended to do?

fw-immunant · 2024-03-11T20:53:40Z

theme/redbox.js

+  console.log("getCodeFromPlayground");
+  const playground =
+    document.getElementsByClassName("ace_text-layer")[0].children;
+  var code = [];
+  for (let i = 0; i < playground.length; i++) {
+    let parentCodeList = {
+      tag: playground[i].tagName,
+      classes: playground[i].classList,
+      styles: playground[i].style,
+    };
+    var line = [];
+    for (let j = 0; j < playground[i].children.length; j++) {
+      console.log(playground[i].innerHTML);
+      let codeList = {
+        tag: playground[i].children[j].tagName,
+        text: playground[i].children[j].innerText,
+        classes: playground[i].children[j].classList,
+        styles: playground[i].children[j].style,
+      };
+      line.push(codeList);
+    }
+    code.push([parentCodeList, line]);
+  }
+  console.log(code);
+  //localStorage.removeItem(window.location.href)
+  localStorage.setItem(window.location.href, JSON.stringify(code));


Again, remove debugging prints and use the public API of the editor rather than accessing the details of its implementation in the DOM.

I don't think I see a reason to store a serialized JSON object in localStorage if the object is just the code--it should be a single string if I understand correctly what information is being persisted.

mani-chand · 2024-03-13T04:53:12Z

I will work on it.

mani-chand · 2024-03-13T17:16:34Z

Hello @djmitche , Its working fine. Just check it once and let me know the changes.

mani-chand · 2024-03-13T18:23:54Z

Hello @djmitche , Its working fine. Just check it once and let me know the changes.

Should I rename the file as addtional.js.
Because `redbox.js' doesn't sound perfect.

djmitche · 2024-03-13T20:57:25Z

Should I rename the file as addtional.js. Because `redbox.js' doesn't sound perfect.

This sounds like a good idea.

I've got a lot of my "regular" job to do this week, before the next version of Chromium is released, so I'll be a little slow to review here. If @fw-immunant has time to take a look, that'd be great -- otherwise I'll get to it by-and-by. Sorry for the delay!

mani-chand · 2024-03-14T03:43:04Z

Should I rename the file as addtional.js. Because `redbox.js' doesn't sound perfect.

This sounds like a good idea.

I've got a lot of my "regular" job to do this week, before the next version of Chromium is released, so I'll be a little slow to review here. If @fw-immunant has time to take a look, that'd be great -- otherwise I'll get to it by-and-by. Sorry for the delay!

Its ok with delay. Take your time and review the PR.

mani-chand · 2024-03-14T04:50:54Z

Should I rename the file as addtional.js. Because `redbox.js' doesn't sound perfect.

This sounds like a good idea.

I've got a lot of my "regular" job to do this week, before the next version of Chromium is released, so I'll be a little slow to review here. If @fw-immunant has time to take a look, that'd be great -- otherwise I'll get to it by-and-by. Sorry for the delay!

All the best for next version of Chromium .

djmitche

Looking good! I made a few suggestions, but the part I don't know is how best to either detect the changes or respond to navigation. I know that onunload is deprecated, and onbeforeunload is for displaying dialogs and not for saving data.

djmitche · 2024-03-18T19:02:17Z

book.toml

@@ -46,7 +46,7 @@ urlcolor = "red"

 [output.html]
 curly-quotes = true
-additional-js = ["theme/speaker-notes.js", "theme/redbox.js"]
+additional-js = ["theme/speaker-notes.js", "theme/additional.js"]


Ah, I thought you meant adding a new JS file for this functionality, rather than moving the existing red-box functionality too. Maybe keep theme/redbox.js and move this new functionality into theme/save-playgrounds.js?

djmitche · 2024-03-18T19:02:21Z

theme/additional.js

+  });
+  localStorage.setItem(window.location.href, JSON.stringify(codes));
+}
+setInterval(getCodeFromPlayground, 900);


This seems like it will wake up the browser frequently even if nothing has changed. I wonder if we could either do this on some event indicating the browser is navigating away (maybe https://developer.mozilla.org/en-US/docs/Web/API/Window/pagehide_event?) or on a timer after the user has modified something in the editor.

djmitche · 2024-03-18T19:04:54Z

theme/additional.js

+    });
+  }
+}
+setCodeToPlayground();


minor point: this is a bit hard to spot here. Please move the whole thing to an IIFE, like the red-box functionality, and put the immediately-evaluated statements at the bottom:

(function savePlaygrounds() { function setCodeToPlayground() .. function getCodeFromPlaground() .. setCodeToPlayground(); setInterval(getCodeFromPlayground, 900); })()

mani-chand · 2024-03-19T04:59:14Z

Hello @djmitche , I have did changes according to the changes required . Let me know any extra changes required. pagehide event is working fine once test it.

djmitche

Great work!! This works just fine for me, and I can see in the debugger that it's setting localStorage reliably, but only when necessary.

As @fw-immunant mentioned, though, we need a way to reset all playgrounds to their default state before this lands -- otherwise instructors will be stuck with modified content from the last time they taught the course.

Maybe we could convert the aspect-ratio icon into an "instructor menu" and put the aspect-ratio item and the reset-playgrounds item in that menu. What do you think?

mani-chand · 2024-03-19T14:39:36Z

Great work!! This works just fine for me, and I can see in the debugger that it's setting localStorage reliably, but only when necessary.

As @fw-immunant mentioned, though, we need a way to reset all playgrounds to their default state before this lands -- otherwise instructors will be stuck with modified content from the last time they taught the course.

Maybe we could convert the aspect-ratio icon into an "instructor menu" and put the aspect-ratio item and the reset-playgrounds item in that menu. What do you think?

Should I make another PR for that or finish it here itself.

djmitche · 2024-03-19T14:48:25Z

I think it will be easiest to do it here, since this PR can't be merged until that's ready.

djmitche

I'm not sure how to solve it, but I noticed that the theme menu (the paintbrush icon) disappears if you click outside the menu, while the instructor menu does not. It'd be great to fix that if it's an easy fix.

djmitche · 2024-03-20T22:13:44Z

theme/redbox.js

+    redBoxButton.innerHTML = "redbox";
+    playgroundStateButton.innerHTML = "reset code";


Let's be more specific about what is reset, and also include a title:

Suggested change

redBoxButton.innerHTML = "redbox";

playgroundStateButton.innerHTML = "reset code";

redBoxButton.innerHTML = "aspect-ratio box";

redBoxButton.title = "Outline the area that fits on one screen while teaching the course.";

playgroundStateButton.innerHTML = "reset all playgrounds";

playgroundStateButton.title = "Reset code in all playgrounds to its original value.";

djmitche · 2024-03-20T22:17:04Z

theme/redbox.js

+
+    redBoxButton.id = "redbox";
+
+    instructorMenuList.style.marginLeft = "55px";


Minor detail: I think this would be easier to read if each item was grouped together, and all its properties set in the same place.

djmitche · 2024-03-20T22:17:38Z

theme/redbox.js

-      hideShowButton.innerHTML = '<i class="fa fa-square-o"></i>';
-    }
-  });
+(function handleInstructor(params) {


I don't think this function needs params!

It probably makes sense to move this to yet another .js file, maybe instructor-menu?

For JS symbols that need to be reachable from other .js files, I think attaching them to window will work, even if it's a bit of a hack:

(function savePlaygrounds() { ... window.resetAllPlaygrounds = function() { .. } })()

(function instructorMenu() { playgroundStateButton.addEventListener("click", () => { ... window.resetAllPlaygrounds(); }); })()

djmitche · 2024-03-20T22:18:43Z

theme/redbox.js

+      playgroundStateItem.lastChild
+    );
+
+    instructorMenu.innerHTML =


Suggested change

instructorMenu.innerHTML =

instructorMenu.title = "Utilities for course instructors";

instructorMenu.innerHTML =

theme/save-playgrounds.js

mani-chand · 2024-03-21T05:37:51Z

Hello @djmitche , Ignore the formatting and check it . Let me know any changes needed.

djmitche

This works great! My remaining comments are more about maintainability, and in particular separation of concerns. That is, the instructor-menu.js file should just set up the instructor menu and call global functions when the menu items are clicked. Everything specific to how those menu items are implemented should be in their own file. For example, the ₹code trick should only appear in save-playgrounds.js.

djmitche · 2024-03-21T20:13:09Z

.github/workflows/build.sh

@@ -40,6 +40,7 @@ mdbook build -d "$dest_dir"

 # Disable the redbox button in built versions of the course
 echo '// Disabled in published builds, see build.sh' > "${dest_dir}/html/theme/redbox.js"
+echo '// Disabled in published builds, see build.sh' > "${dest_dir}/html/theme/instructor-menu.js"


Oh! I think we don't want this anymore, as we will need the "reset playgrounds" option in the published build. @mgeisler now that we have all of this in a sub-menu, what do you think?

djmitche · 2024-03-21T20:19:30Z

theme/redbox.js

@@ -1,4 +1,4 @@
-(function redBoxButton() {
+function redBoxButton() {


I think this should still be an IIFE, but at the end should have something like

... window.redboxButtonClicked = redboxButtonClicked; })();

Similarly, save-playgrounds.js would have

... window.resetPlaygroundsClicked = resetPlaygroundsClicked; })();

Then instructor-menu.js can have

redboxButton.addEventListener("click", e => window.redboxButtonClicked(e)); playgroundStateButton.addEventListener("click", e => window.resetPlaygroundsClicked(e));

with all of the implementation of what to do when those buttons are clicked appearing in the appropriate file.

mani-chand · 2024-03-22T05:00:14Z

Hello @djmitche , check and Let me know any changes needed.

djmitche

I think this looks good! This was a lot of work and a lot of rounds of review, but I think the result is going to make teaching the course quite a bit better.

I'd like to get some feedback from @mgeisler regarding keeping the menu in builds before merging it.

A few minor points that could be addressed in a later PR if you'd prefer:

There's no feedback on clicking the "reset all playgrounds" to indicate that all playgrounds were reset. In fact, if one appears on the current slide, its contents aren't reset. I know alert() is lame, but maybe this is a good use-case for it?
It feels weird that the menu stays visible after clicking one of the items. Maybe hide it in the "click" handlers?

mani-chand · 2024-03-22T12:48:09Z

I think this looks good! This was a lot of work and a lot of rounds of review, but I think the result is going to make teaching the course quite a bit better.

I'd like to get some feedback from @mgeisler regarding keeping the menu in builds before merging it.

A few minor points that could be addressed in a later PR if you'd prefer:

There's no feedback on clicking the "reset all playgrounds" to indicate that all playgrounds were reset. In fact, if one appears on the current slide, its contents aren't reset. I know alert() is lame, but maybe this is a good use-case for it?

It feels weird that the menu stays visible after clicking one of the items. Maybe hide it in the "click" handlers?

I can do 2nd point. If you say it should hide on clicking one option it is possible. I will do it and push it.

mani-chand · 2024-03-22T12:48:47Z

I think this looks good! This was a lot of work and a lot of rounds of review, but I think the result is going to make teaching the course quite a bit better.

I'd like to get some feedback from @mgeisler regarding keeping the menu in builds before merging it.

A few minor points that could be addressed in a later PR if you'd prefer:

There's no feedback on clicking the "reset all playgrounds" to indicate that all playgrounds were reset. In fact, if one appears on the current slide, its contents aren't reset. I know alert() is lame, but maybe this is a good use-case for it?

It feels weird that the menu stays visible after clicking one of the items. Maybe hide it in the "click" handlers?

1st point : confirm works fine I think.

mani-chand · 2024-03-22T12:49:16Z

I think this looks good! This was a lot of work and a lot of rounds of review, but I think the result is going to make teaching the course quite a bit better.

I'd like to get some feedback from @mgeisler regarding keeping the menu in builds before merging it.

A few minor points that could be addressed in a later PR if you'd prefer:

There's no feedback on clicking the "reset all playgrounds" to indicate that all playgrounds were reset. In fact, if one appears on the current slide, its contents aren't reset. I know alert() is lame, but maybe this is a good use-case for it?

It feels weird that the menu stays visible after clicking one of the items. Maybe hide it in the "click" handlers?

for 1st point confirm works fine I think.

mani-chand · 2024-03-22T13:04:31Z

I think this looks good! This was a lot of work and a lot of rounds of review, but I think the result is going to make teaching the course quite a bit better.

I'd like to get some feedback from @mgeisler regarding keeping the menu in builds before merging it.

A few minor points that could be addressed in a later PR if you'd prefer:

There's no feedback on clicking the "reset all playgrounds" to indicate that all playgrounds were reset. In fact, if one appears on the current slide, its contents aren't reset. I know alert() is lame, but maybe this is a good use-case for it?

It feels weird that the menu stays visible after clicking one of the items. Maybe hide it in the "click" handlers?

@djmitche , Once can you test it again.
It feels weird that the menu stays visible after clicking one of the items. Maybe hide it in the "click" handlers? - Completed/Solved.

djmitche · 2024-03-22T13:29:29Z

Nice! That might also address the feedback issue. I'm happy to merge this as-is, if others are OK.

mgeisler · 2024-03-22T15:30:44Z

theme/save-playgrounds.js

+      let code = editor.getValue();
+      codes.push(code);
+    });
+    localStorage.setItem(`${window.location.href}₹code`, JSON.stringify(codes));


Has it already been discussed what the ₹ character is doing here? It looks a bit strange to me 😄

Have you considered using just the pathname instead of the full URL? Since local storage is different for different hosts, I don't think we need the full URL here.

Oh, if the ₹ is necessary somehow, then I would suggest creating a new function to construct the key. You can then document the use there — and also call it from both places that need this key.

Oh, if the ₹ is necessary somehow, then I would suggest creating a new function to construct the key. You can then document the use there — and also call it from both places that need this key.

Sorry, I didn't understand creating of "another function for key creating".

Hey @mani-chand, I'm suggesting creating a one-line function which returns the correct key for the current page. Something like

function localStorageKey() { // The '₹' here is used for ... return `${window.location.href}₹code` }

You should then explain what ₹ does here 🙂

mgeisler · 2024-03-22T15:34:01Z

theme/instructor-menu.js

@@ -0,0 +1,71 @@
+(function handleInstructor() {
+  function handleInstructorMenu() {


Did you discuss simply adding this to the theme instead? Since this will be a permanent thing on all pages, it should go into index.hbs or head.hbs.

That would simplify the code a lot since you can just write out the HTML directly.

Why was this not done for the red box, you might ask? Because that was not supposed to be rendered for people looking at the course in the published version.

mgeisler · 2024-03-22T15:48:14Z

I think this looks good! This was a lot of work and a lot of rounds of review, but I think the result is going to make teaching the course quite a bit better.

I'd like to get some feedback from @mgeisler regarding keeping the menu in builds before merging it.

My plans for writing a new theme are basically on pause right now, so I guess it's fine to take away some more horizontal space now.

So feel free to merge this and then we can improve it further afterwards.

One potential way of improving it would be to reset all playgrounds the same way as a single playground is reset:

The button with the rotating arrow in the top-right resets the playground. Perhaps you could let Ctrl-click reset all playgrounds?

Alternatively, we can create a page in the course which embeds the necessary JavaScript to reset all playgrounds. Call it an "Instructors toolbox" page. It could be under Running the Course since that segment is already mostly for the instructor.

This can all be for later, I think it's more important to get this cool new functionality published 🎉 After merging it, we can update all the exercises to have the editable attribute so that people can edit them inline again.

requested changes made

When browing around on https://google.github.io/comprehensive-rust/, I sooner or later end up in a state where the local storage has `[]` stored for a page with one or more playgrounds. The effect of this is that the code is removed from the page! I am not sure why this happens, but I’m afraid the code here needs more testing. I’m teaching a class Monday morning, so I’ll disable the code from #1917 for now.

saving playground code state

6fdae32

djmitche self-requested a review March 11, 2024 17:15

mani-chand marked this pull request as draft March 11, 2024 17:17

Formatted code

65e4327

djmitche requested a review from fw-immunant March 11, 2024 19:44

djmitche mentioned this pull request Mar 11, 2024

Keep state of the Rust Playgrounds when navigating between slides #1476

Open

fw-immunant previously requested changes Mar 11, 2024

View reviewed changes

mani-chand added 2 commits March 13, 2024 22:28

Saving code of playground by local storage using ace api.

61403a5

Formatted

58b36a1

mani-chand marked this pull request as ready for review March 13, 2024 17:00

mani-chand added 2 commits March 13, 2024 22:34

updated setCodeToPlayground function call

9a42d76

code Formatted

23f7592

mani-chand requested a review from fw-immunant March 13, 2024 17:10

Renamed redbox.js to additional.js

5a27433

djmitche reviewed Mar 18, 2024

View reviewed changes

mani-chand added 3 commits March 19, 2024 09:53

Updated get and set code function for playground.

7c51897

code formatted

8fea0af

code formatted

55729b0

mani-chand requested a review from djmitche March 19, 2024 07:37

djmitche reviewed Mar 19, 2024

View reviewed changes

djmitche requested changes Mar 20, 2024

View reviewed changes

mani-chand added 3 commits March 21, 2024 10:36

Requested changes

9d03037

disable instructor-menu in build.sh

ed05702

Formatted redbox css

138fbd7

mani-chand requested a review from djmitche March 21, 2024 05:36

Code formatted

28fbdd9

djmitche requested changes Mar 21, 2024

View reviewed changes

Requested changes

8889a48

mani-chand requested a review from djmitche March 22, 2024 04:59

djmitche approved these changes Mar 22, 2024

View reviewed changes

instructor menu Hide event updated

7a0de16

mani-chand requested a review from djmitche March 22, 2024 13:02

djmitche requested a review from mgeisler March 22, 2024 13:29

mgeisler reviewed Mar 22, 2024

View reviewed changes

djmitche merged commit b7a6e3b into google:main Mar 22, 2024
32 checks passed

mani-chand deleted the #1476_issue branch March 22, 2024 19:10

mgeisler mentioned this pull request Mar 24, 2024

Temporarily disable saving playground state #1935

Merged

mani-chand mentioned this pull request Apr 7, 2024

Saving the playground state in local storage #1942

Merged

		redBoxButton.innerHTML = "redbox";
		playgroundStateButton.innerHTML = "reset code";

-    redBoxButton.innerHTML = "redbox";
-    playgroundStateButton.innerHTML = "reset code";
+    redBoxButton.innerHTML = "aspect-ratio box";
+    redBoxButton.title = "Outline the area that fits on one screen while teaching the course.";
+    playgroundStateButton.innerHTML = "reset all playgrounds";
+    playgroundStateButton.title = "Reset code in all playgrounds to its original value.";


		redBoxButton.id = "redbox";

		instructorMenuList.style.marginLeft = "55px";

	instructorMenu.innerHTML =
	instructorMenu.title = "Utilities for course instructors";
	instructorMenu.innerHTML =

		@@ -1,4 +1,4 @@
		(function redBoxButton() {
		function redBoxButton() {

		@@ -0,0 +1,71 @@
		(function handleInstructor() {
		function handleInstructorMenu() {

Saving playground code state #1917

Saving playground code state #1917

Conversation

mani-chand commented Mar 11, 2024

djmitche commented Mar 11, 2024

fw-immunant left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mani-chand commented Mar 13, 2024

mani-chand commented Mar 13, 2024

mani-chand commented Mar 13, 2024 • edited Loading

djmitche commented Mar 13, 2024

mani-chand commented Mar 14, 2024

mani-chand commented Mar 14, 2024

djmitche left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mani-chand commented Mar 19, 2024

djmitche left a comment

Choose a reason for hiding this comment

mani-chand commented Mar 19, 2024

djmitche commented Mar 19, 2024

djmitche left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mani-chand commented Mar 21, 2024

djmitche left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mani-chand commented Mar 22, 2024

djmitche left a comment

Choose a reason for hiding this comment

mani-chand commented Mar 22, 2024

mani-chand commented Mar 22, 2024 • edited Loading

mani-chand commented Mar 22, 2024

mani-chand commented Mar 22, 2024 • edited Loading

djmitche commented Mar 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgeisler commented Mar 22, 2024

mani-chand commented Mar 13, 2024 •

edited

Loading

mani-chand commented Mar 22, 2024 •

edited

Loading

mani-chand commented Mar 22, 2024 •

edited

Loading