Closed gwhitney closed 3 months ago
OK, @katestange @Vectornaut just managed to finish cleaning this up (it's getting more complicated with each PR in sequence). I know of at least four five items that we at least need to consider whether we want to address before merging this PR, in addition to the usual code and behavior review of which I have not done yet:
[x] This PR removes two tests from browserCaching.ts but does not add back in tests for the new methods. I think this is a must.
[x] The former BundleCards in the original UI automatically stopped the previews after a short fixed amount of time. Should this do something similar?
[x] The functionality/role and even code of FeaturedCard
and SpecimenCard
seem very similar, suggesting there is redundancy/non-DRYness going on here -- ideally we should clean that up before merging. [Have pushed a commit combining these components into one.]
[Deferred] When you load a specimen from the gallery and then change a parameter, it still just appears on screen as the specimen of that name, but it now actually differs from the specimen saved under that name. There is no indication that it's actually a different specimen than the one named in the top bar. Is that acceptable behavior for now, or should we add an indication that what's currently shown is different what's been saved? Such an indication could be as subtle as the "save" button is hidden when what is shown matches what's currently saved under that name but visible (and perhaps slightly more emphatic than it is now) when there is some difference between the shown name and what's currently saved under that name.
[Deferred] Kaldis reminds us that the original Delft design was to have separate Saved and Featured tabs in the Gallery view, and then with each tab, have categories of specimens stacked vertically (e.g. by visualizer type). Do we want to enhance the implementation in that way now, or merge this PR without that feature and leave the more fine-grained Gallery as an issue for a possible future PR?
Have at it, I will come back and look more when I am able.
Hmm, well the problem is that the cursor is now a hand over the label, but you can't click on the label. We'd need to make everywhere we have a pointerhand clickable, I think.
OK, I will make those clicks load the specimen and push a commit for that.
Right now if you click "Gallery" and then click a specimen and get to the scope, then click the browser back button, nothing happens (you'd expect to go back to gallery).
Right now I have 14 visualizers showing in the Gallery page (between featured and cached). It is causing my browser to report an unresponsive page. I think maybe we really do need to limit the live rendering on the Gallery page?
Right now if you click "Gallery" and then click a specimen and get to the scope, then click the browser back button, nothing happens (you'd expect to go back to gallery).
Hmm, works for me. What browser are you using? Try closing that browser window/tab and restarting the dev server and then going back to the page and then doing some clicks and backs. Does it still not work?
Right now I have 14 visualizers showing in the Gallery page (between featured and cached). It is causing my browser to report an unresponsive page. I think maybe we really do need to limit the live rendering on the Gallery page?
Yes, I agree we need to do something. In general, I feel like Numberscope has become a real resource hog, more so than it used to be. I fear that maybe some Specimens that are not showing are still running (i.e., have not been cleaned up properly). Does anyone know a way of checking? Anyhow, for the immediate issue of the gallery, should we just call .stop() on the thumbnails after 1 second of runtime like we used to do? Longer/shorter time, or different way to limit resources?
Right now if you click "Gallery" and then click a specimen and get to the scope, then click the browser back button, nothing happens (you'd expect to go back to gallery).
Hmm, works for me. What browser are you using? Try closing that browser window/tab and restarting the dev server and then going back to the page and then doing some clicks and backs. Does it still not work?
Oh, hmm, it works on a fresh one. My browser seems to have gotten all lagged up -- maybe the 14 cards isn't the problem, but there's something accumulating. When I click on any visualizer and it goes to scope, all other visualizers should be dead and gone, right? I'm seeing very slow rendering now, as if they are still running in the background. This is on Chromium.
Right now I have 14 visualizers showing in the Gallery page (between featured and cached). It is causing my browser to report an unresponsive page. I think maybe we really do need to limit the live rendering on the Gallery page?
Yes, I agree we need to do something. In general, I feel like Numberscope has become a real resource hog, more so than it used to be. I fear that maybe some Specimens that are not showing are still running (i.e., have not been cleaned up properly). Does anyone know a way of checking? Anyhow, for the immediate issue of the gallery, should we just call .stop() on the thumbnails after 1 second of runtime like we used to do? Longer/shorter time, or different way to limit resources?
Yes, I think we should call .stop() after some time (for lack of a better idea). Whoever is coding that should just experiment to find a good timing. And yes, we must definitely be not cleaning something up properly, because the instance I've been playing with has just ground to a halt now after experimenting and playing around for a while. It's frozen.
And yes, we must definitely be not cleaning something up properly, because the instance I've been playing with has just ground to a halt now after experimenting and playing around for a while. It's frozen.
Yeah, I was afraid of that. Anyone have any ideas how to track down where the zombie visualizers are being leaked from?
And yes, we must definitely be not cleaning something up properly, because the instance I've been playing with has just ground to a halt now after experimenting and playing around for a while. It's frozen.
Yeah, I was afraid of that. Anyone have any ideas how to track down where the zombie visualizers are being leaked from?
OK, I have limited the drawing time in the Gallery to four seconds, and I think I have greatly reduced the degree to which visualizers "leak" and keep running even after they are no longer needed. Please play around with an instance as extensively as you did before and see if that grinding to a halt is ameliorated. Thanks!
OK, I have limited the drawing time in the Gallery to four seconds, and I think I have greatly reduced the degree to which visualizers "leak" and keep running even after they are no longer needed. Please play around with an instance as extensively as you did before and see if that grinding to a halt is ameliorated. Thanks!
What did you find/do? It is much better now, I beat specifically on it like a drum for a while, and I can get the fan to spin up on my computer but it resolves and stops when I stop banging on it, within a few seconds. Is that because the gallery thumbnails all stop after 4 seconds despite living on when you go to the scope, or are they dead now when you go to the scope?
OK, I have limited the drawing time in the Gallery to four seconds, and I think I have greatly reduced the degree to which visualizers "leak" and keep running even after they are no longer needed. Please play around with an instance as extensively as you did before and see if that grinding to a halt is ameliorated. Thanks!
What did you find/do? It is much better now, I beat specifically on it like a drum for a while, and I can get the fan to spin up on my computer but it resolves and stops when I stop banging on it, within a few seconds. Is that because the gallery thumbnails all stop after 4 seconds despite living on when you go to the scope, or are they dead now when you go to the scope?
Glad to hear this is better for you! I found that Visualizer.depart()
was never being called when components were cleaning themselves up (in the current design, .depart() is needed because otherwise the Visualizer thinks maybe it will be made visible again, so it keeps computing). So I added several places where Visualizer.depart() is called; not certain I got them all. So that should definitely help with the slow accumulation of processor usage. In particular, I think that all of the visualizers in the Gallery are being cleaned up and prevented from any further resource usage when you go to the scope.
And then on the Gallery page when a gazillion visualizers are showing at once, pausing each one after 4 seconds prevents that one page from just consuming your processor indefinitely while it is displaying. I'm not going to worry about processor usage further for this PR unless someone notices a problem.
Why is the word "empty" here?
Great, I'm going to hide as resolved all the comments to do with visualizer cleanup.
Why is the word "empty" here?
Yeah, that seems like a bug. Will add it to the list to look into. I think it is because it is extracting the sequence name before the sequence is fully initialized.
Why is the word "empty" here?
Yeah, that seems like a bug. Will add it to the list to look into. I think it is because it is extracting the sequence name before the sequence is fully initialized.
OK, I believe I have this fixed; please give it a try.
Why is the word "empty" here? Yeah, that seems like a bug. Will add it to the list to look into. I think it is because it is extracting the sequence name before the sequence is fully initialized.
OK, I believe I have this fixed; please give it a try.
Works on my machine!
Works on my machine!
OK, then to summarize the remaining points in this PR:
Someone, probably Glen but open to help needs to:
And at the upcoming meeting we need to discuss how to proceed with the following aspects of this PR:
I am not aware of anything beyond the above, although anyone should feel free to bring up other things.
Browser caching tests pass for me.
OK, I have cleaned up the Paramable interface so we don't have several repeated lists of calls to Paramable functions; the logic is now all in Paramable.ts itself. As this was a rather bigger change than most of the commits added to this PR, I'd appreciate some amount of beating on this before we merge. Thanks! (There should be no operational changes based on this commit, it was a pure refactor to avoid redundant code.)
Consensus on remaining items: A) Leave opaque URLS but make defineFeatured an array of base64s, with the names as comments. Readable URLs will be left as a later issue. B) Change indicator will wait for a post-Delft todo, but will be required before ui2 -> main C) Articulated gallery will wait for a later issue.
OK, @Vectornaut (and @katestange) with the push I just made all that remains is for you to beat on this a bit more, look over anything you want, and merge it. Then I will prep the next PR. Thanks!
It seems to be behaving as expected. @Vectornaut I'll let you play with it and merge when you are ready.
I'll let you play with it and merge when you are ready.
Nice! I should be able to try it sometime in the next two or three hours.
I can't tell whether this PR caused this issue, because I can't access formula-defined sequences in previous versions.
sign(sin())
.@gwhitney, @katestange, should we change the trash button behavior mentioned above before merging this? (Should we change it at all?)
@gwhitney, @katestange, should we change the trash button behavior mentioned above before merging this? (Should we change it at all?)
Yes, that's a bug I introduced. Good catch. I will push a fix as soon as I can get to it.
The formula issue is more general, has been around since their first reactive PR, and we are going to fix after all their PRs but before merging ui2 -> main. Thanks!
OK, should be fixed; anyone should feel free to squash-merge this to ui2 if it works on your end. Thanks! Then I will prepare a cleaned version of the sequence/visualizer switcher.
By submitting this PR, I am indicating to the Numberscope maintainers that I have read and understood the contributing guidelines and that this PR follows those guidelines to the best of my knowledge. I have also read the pull request checklist and followed the instructions therein.
Thumbnails
Gallery
Scope
Supersedes #358.