Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Epic dynamic diagram 3 #93

Merged
merged 6 commits into from
Oct 2, 2023
Merged

Epic dynamic diagram 3 #93

merged 6 commits into from
Oct 2, 2023

Conversation

fdodino
Copy link
Contributor

@fdodino fdodino commented Sep 30, 2023

Mejoras

  • Poner un toggle para que redibuje (o no) los objetos cuando se corra un comando
  • Agregar un iconito para que quede claro que es Dark/Mode
  • Fix de las referencias de las colecciones que aparecen con un ancho zarpado
  • Hice una prueba de volumen con 150 nodos y funcionó muy bien (300 ms). No necesitamos por el momento hacer optimizaciones. De hecho con 150 nodos si activás el "pin" (la última opción del diagrama, que tiene el pinchecito) tarda 7 ms!! en agregar nuevos elementos. El re-render del layout ahí se vuelve pesado, eso habría que documentarlo en la nueva página porque es importante saberlo, aunque la verdad no veo que tenga sentido meter más de 100 nodos...

image

Queda pendiente: "Una constante del REPL no figura con el candadito" pero para eso necesito pairear un toque con vos Nahue y entender así mejor lo de los scopes.

@fdodino fdodino marked this pull request as ready for review September 30, 2023 23:16
@fdodino fdodino added the componente: diagrama-dinamico Muestra los objetos en el diagrama label Sep 30, 2023
Copy link
Contributor

@PalumboN PalumboN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pioliiixxxxx 👍

Comment on lines 136 to 146
function reloadDiagram(elements) {
const initTime = Date.now()

currentElements = [...elements]
changeElementsMode()
const ids = elements.map((element) => element.data.id)
cy.filter((element) => !ids.includes(element.id())).remove()

console.info('Dynamic Diagram - Performance metrics')
console.info('- Removing old elements', Date.now() - initTime)
const midTime = Date.now()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cheeee y no podemos dejar estas métricas ocultas? Y activarlas en un modo DEBUG?

Me interesan y me gustaría que no sea paja activarlas para bajar el costo de hacer experimentos :)

Copy link
Contributor Author

@fdodino fdodino Oct 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inicialmente yo pensaba dejarlas: yo le apunté a ver si con muchos elementos usar un mapa mejoraba la performance. Pero lo que más cuesta, más allá de la búsqueda dentro del array, es renderizar dentro del canvas muchos nodos. Por eso con 150 nodos, si activás el "pin" nuevo que le pusimos, anda rápido y 0 problema (hay que avisar a les usuaries de alguna manera, le pondría un tooltip que diga eso a futuro).

El otro tema es que ponerle esa info ensucia el código (lo vuelve menos mantenible) y es medio una paja que no se puede reutilizar lo de las métricas de performance que están en el server de wollok-lsp-ide (esto es wollok-ts-cli y encima en un navegador). Por eso al final decidí volarlo y si en algún momento queremos pensamos un componente que pueda agregarle métricas.

@fdodino fdodino merged commit 6db2a5b into master Oct 2, 2023
@fdodino fdodino deleted the epic-dynamic-diagram-3 branch October 2, 2023 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
componente: diagrama-dinamico Muestra los objetos en el diagrama
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants