HubNet Web, or something... #416
Conversation
There was a problem hiding this comment.
This is awesome. It's a lot of fun to play with. Congratulations!
I left some comments in the code, mostly the replies to your TODO items. I didn't test as deeply as I'd like (especially with authoring), as I wanted to get this back to you. I'll try to carve some time to get you more feedback next week.
Besides the code comments, I found a grand total of two items I think need addressing before merge:
- In the regular model launch page (not HNW), the Search the models library and Upload a model boxes are now stacked vertically instead of horizontally.
- There might be a regression with histogram display for highcharts. The bars look much wider than before. Crystallization Basic is an easy model to check.
I found a lot of small things, some bug-ish, some just improvements. I don't consider handling any of these as necessary to merge. If you'd prefer I move these into issue(s) in the HubNet Web repo, just let me know. I probably used the word "nice" way too often, my apologies.
When hosting, joining, or authoring, the page title is unset, so just the URL shows.
Hosting and Joining
The hotkey combo to enter authoring mode is still available as both host and joiner. The help ? hotkey, too, which probably isn't useful as is.
There is a pretty big section of whitespace on the left side of the host's model in the iframe (screenshot attached). I checked a few different models as host and they all seemed to have the same large space there to varying degrees.
When hosting a session, I don't think I can see what my chosen session name was. The share link is there, so this seems like a minor thing, but it makes sense to me to be able to see it.
I got confused about the lack of the command center. Once I found it it made sense. Not sure there is anything to be done there to help people find it when they're new to the system, just pointing it out. I guess maybe a more meaningful label than Menu?
The setup/go buttons at the bottom feel weird to me, I'm just so used to them being near the top of a model. I think it also throws me off that its styled differently than the widget buttons, so I don't immediately think of them as model actions.
Is there an option to not have the extra "student" session when I'm the host? I can see some hosts not wanted to have it sitting around.
As the host, scrolling around the models iframe is slightly annoying. It's hard to get exactly what I want on the screen. 1) Just because it's a scrolly window inside a scrolling window, which is always a little wonky. But 2) it's also hard to see the example student's stuff at the same time. Possibly silly idea: Instead of starting the student session, could you have a button to open a new window (or tab) with the student stuff? That way the browser windows can be positioned however is most useful for the host.
Authoring
When I open Authoring and then Create a model from scratch, then switch from Code to teacher or student, the widget editor boxes popup offset way to the right, as though they were being displaced by the (now invisible) prior items.
It might be nice if I could auto-generate some of the procedures for the actions in the roles, mostly so I don't have to look up what the arguments are. A way to jump to the code for an already-selected procedure directly from the role would be nice, too.
It would be very nice if I could see the code and role(s) side-by-side.
Having the option to load and edit the already-included models would be very handy, so I can see how things work in other models.
Needing to scroll the Code and role UI windows/iframes in the authoring section is a bit of a pain. I think I'd prefer if the iframe contents were just max-size so I can do the scrolling in the main window.
build.sbt
Outdated
| "com.typesafe.play" %% "play-iteratees" % "2.6.1", | ||
| "com.typesafe.akka" %% "akka-testkit" % "2.5.15" % "test", | ||
| "org.scalatestplus.play" %% "scalatestplus-play" % "3.1.2" % "test" | ||
| ) |
There was a problem hiding this comment.
Seems like these library dependencies got duplicated with another set, probably during the rebase?
build.sbt
Outdated
| @@ -36,7 +36,18 @@ lazy val root = (project in file(".")) | |||
| TestAssets / JsEngineKeys.npmNodeModules := Nil | |||
| ) | |||
|
|
|||
| val tortoiseVersion = "1.0-2fd0f5a" | |||
| val tortoiseVersion = "1.0-44dea86" | |||
There was a problem hiding this comment.
With the merge of the Tortoise PR and the publishing of the latest for the NetLogo Web release, I think this can now just use what's currently on master.
| Tortoise.fromNlogoSync( | ||
| source | ||
| , modelContainer | ||
| , "en_us" # TODO: Where am I supposed to get this? |
There was a problem hiding this comment.
You can leave as-is for now. There is pending work to properly pull it from the user's browser settings or their site setting overrides and we can update this when that work goes in as well.
| , modelContainer | ||
| , "en_us" # TODO: Where am I supposed to get this? | ||
| , false # TODO: Eh? | ||
| , null # TODO: What is `getWorkInProgress` for? |
There was a problem hiding this comment.
The getWorkInProgress() is for getting any change a user has to a model saved in local storage, so it can be loaded instead of the regular model code. The boolean parameter above is related, it indicates if the user is reverting their work-in-progress model changes back to the original model code. Neither of these seem relevant for HNW.
| , false # TODO: Eh? | ||
| , null # TODO: What is `getWorkInProgress` for? | ||
| , handleCR | ||
| , [] # TODO: Should I have rewriters? |
There was a problem hiding this comment.
The rewriter.coffee contains a bit more info on these, but they can be used to alter NetLogo code on load (from nlogo source) or on recompile, as well as on export (to make sure exported code works without the rewriters). NetTango uses this functionality to insert its code without having to munge the code in the code tab in a way the user could interfere with. I'm not sure if this would be useful for HNW.
| , null # TODO: What is `getWorkInProgress` for? | ||
| , handleCR | ||
| , [] # TODO: Should I have rewriters? | ||
| , [] # TODO: Should I have listeners? |
There was a problem hiding this comment.
I did refactor the error/alert display system to use the event listener system, so that may be relevant to HNW. The AlertDisplay is wired up in simulation.js and listener-events.coffee has more info on what can be listened for. Just let me know if you have questions on it.
|
Alright, cool. Thanks for your feedback! A lot of these changes nudged me to solve things that I'd been meaning to address for a while now, and helped things to look and function in a way that I'm prouder of. If you update to the latest version of this branch and the HNW repo's Fixed
Sometimes, interns do unexpected things to our CSS. And, sometimes, supervisors don't catch those things. 😄 697b75e
Good catch. This was a big ole goof on my part. I was ignoring the values for
The fix for this is over the other repo. NetLogo/HubNet-Web@6773093
Renamed to "Tools". NetLogo/HubNet-Web@159a61b
Yeah, interface editing, at the very least, needed to be disabled. 'Tis done. 9c976d2
Thanks for nudging me on this one. This issue had actually been a perpetual source of annoyance for me, over the years of working on this. And, on multiple occasions, I looked into it and arrived at the conclusion that I either couldn't or shouldn't do anything about it, because it seemed like things were working about as well as they could, as hacky and crappy as they were. But, looking upon it with fresh eyes, I was actually able to drastically improve things this time. 51330e8 Arguably the code in NLW for resizing the model's
Fixed in 9326e86
Yeah, this was a good change. But note that, while I was able to get rid of one level of scrollbars, I wasn't able to get rid of the scrollbar inside of the editor, itself. It seems that, when I try to do that, I get stuck in an infinite loop of CodeMirror consuming more and more of the page's height, rather than just sticking to the amount that it needs. 7723460 Deferred
Won't Fix
After some thought, I'm not sure I want to act on this at the moment. I agree that the current design is clunky and unsightly. And my original conception for the design was that the host would have only their own client UI, and not also one for the secondary role. I only put both of the UIs on the same page because it was convenient during development. But it proved to be so incredibly convenient that I'm no longer even convinced that it's a bad idea. I'd like to see what the reception to it will be during the beta. More Information Required
If you click "More Stats", the title of the modal that pops up is "Session Stats: <your title>". Do you have a different place you'd prefer to see it?
How might things be done differently, such that they might feel more intuitive to you? |
There was a problem hiding this comment.
Thank you for the updates! Everything is looking/running well.
GitHub is reporting some merge conflicts, likely due to some changes that landed while you were working on this. Just want to make sure you saw them.
I found a couple of things while testing; neither of these I consider show stoppers to merge this PR, but thought I'd report them:
- The "Start Over" button at the top when authoring an activity seems strangely prominent for something I probably wouldn't use that often. It might make more sense being in line with the other buttons (download/test) at the bottom. It also doesn't give a warning about losing work in progress if I click it.
- Current in a Wire didn't seem to be working. The view for the clients was partial, but stretched to the full canvas size (not sure if that is how it's to be). But more importantly (I think) the model didn't seem to do anything (no electrons move around) and I saw some uncaught runtime errors in the dev console, example:
message: "SLICES breed does not own variable OVERRIDES"
For more info items:
"Re: Visibility of session name as host" If you click "More Stats", the title of the modal that pops up is "Session Stats: ". Do you have a different place you'd prefer to see it?
I guess I expected to see it directly on the hosting page somewhere, maybe along with the title of the activity? But again, I don't think it makes much difference. Maybe if someone wanted to host multiple of the same activity at once?
"Re: Setup/Go button position" How might things be done differently, such that they might feel more intuitive to you?
After staring at the host page a bit, I think it'd make more sense to me if they were to the left of the speed slider, above the host's role window. If that's too difficult to accomodate with the current layout, just having them above the host window somewhere might be more obvious. But this is definitely a subjective thing so getting other opinions might help.
This mainly affected HNW, where a session could be launched and then shut down before it was used. The consequence was that, once you loaded a full session, you would actually have two event loops running with `rAF`, each one pulling events out of `Updater` and preventing the other from receiving and rendering them. This, of course, would not happen deterministically, so it was tons of fun to figure out what was going on.
Co-authored-by: Christopher Song <chris4728.song2001@gmail.com> Chris's contributions focused on the HNW UI, especially modal dialogs and the dialog box "drawer" (containing the Command Center, etc.)
It's been a long, long time coming...
It can sometimes cause plots to go blank, when a plot is reset without any series/pens One example where this can happen is Disease, where all of the plot pens are temporary pens
So they don't accidentally lose work
Those should be resolved now.
Good point. Fixed.
The page will now warn you when clicking this button. It will also warn you when closing an authoring tab.
I was able to see this error on the public website for HNW, but never locally. I'm guessing it was some old bug that I have since resolved.
Resolved.
Resolved. I think that should be just about everything. I tackled those issues (and more), and dumped my changes into commits labeled "Temp". I've left those commits small and separate, so that you can look at what's changed, but I'll squash those down into the mega-commit when this PR gets a final sign-off. |
There was a problem hiding this comment.
Code changes all look good to me.
One small idea after testing the "Are you sure?" nag for authoring: We might want to add the same for hosting sessions. If you have a couple dozen people in the middle of an activity it'd be a shame to have to get everyone re-joined and things running again because of an errant click.
One small model issue I found: In Bird Breeders the instruction buttons send the output to the command console instead of the output widget.
But I don't consider either of those necessary for merging. Thanks for all your work on this!
|
It's merged now. Good catches, on those bugs! I'll open tickets for those in the HNW repository. Thanks for your diligent testing and patience with getting this through. 🙇♂️ |
Okee, here's a microscopically enormous pull request for adding HubNet Web. There's a lot that probably needs cleaning up.
The real meat of this PR is, of course, the "Major/HNW/Feature: Add HNW and HNW Authoring" commit, which has a ton of stuff in it. Surrounding that commit are a bunch of things that I fixed while working on HNW, but that I believe improve Galapagos more broadly.
Especially note the TODOs in
app/assets/javascripts/beak/hnw/host/load-model.coffeeandapp/assets/javascripts/beak/tortoise.coffee, as I think I need to be filled in on the purpose of a number of the new parameters that are used to launch models in NLW, especially rewriters, listeners, andgetWorkInProgress.Also, please note that this branch uses a Tortoise
.jaris whose code is not yet integrated into Tortoise'smasterbranch. That code will be in another (much smaller) PR, mostly pertaining to changes that I made to the compiler's API, so that plots can be compiled individually.