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

Proceds blockly parte 2 #2

Merged
merged 25 commits into from
Nov 28, 2024
Merged

Proceds blockly parte 2 #2

merged 25 commits into from
Nov 28, 2024

Conversation

dlopezalvas
Copy link
Contributor

@dlopezalvas dlopezalvas commented Jul 17, 2024

Related Program-AR/pilas-bloques-app#248

pilas-bloques-app: Program-AR/pilas-bloques-app#316

Lo que falta hacer:

  • Arreglar que al crear un procedimiento con parámetros, si se crea el caller y luego se le cambian los nombres a los parámetros, no se cambian en los callers y tampoco se cambian en el procedimiento en si. Como se ve en el video, pareciera ser un tema de que no está actualizando la parte visual de los callers, porque al crear un nuevo caller después de cambiar los nombres, si lo crea con los nombres nuevos.
    Grabación de pantalla desde 17-07-24 11_06_56

  • Al crear el caller de un parámetro los crea con una flechita para abrir un menu:
    imagen
    Hay que cambiarlo para que este como ahora en producción:
    imagen

  • Los callers no aparecen en el toolbox al crear el procedimiento, puede que sea lo que charlamos ayer en la reunión que toma que el toolbox tiene otro workspace, quizás hay que actualizarlo poniendole los bloques del main workspace.

  • Queda en el init pasar las funciones customContextMenu y domToMutation que no llegue ni a arrancarlas. Habría que ver bien qué es lo que hacen y tomar solo las cosas que nos interesan, hay varias cosas de proceds-blockly que no pase acá porque no tenía sentido para lo que vamos a usar, seguí la idea de lo minimo necesario, quizás en un futuro se puede ver el resto.

  • Una vez que esté arreglado los problemas que mencioné antes, quedaría hacer las pruebas que dijo Alf, yo un poco probe borrar procedimientos y se borran también los callers, eso va bien, pero me gustaría saber qué pasa con el xml de la solución cuando se borran o cambian nombres, es decir ver que "por atrás" se esté haciendo todo bien y que no sea que tenemos solo lo visual bien. También probaría el código que genera una solución con nuestros bloques y la compararía con la versión de producción hoy.

  • Arreglar en pilas-bloques-app el import, ahora mismo hay un @ts-ignore antes del import de ProcedsBlocklyInit, porque estaba tirando error por tipos (no tiene archivo de tipos este paquete), no llegamos a solucionarlo con Ro y lo parcheamos así, pero no debería quedar con el ignore.

  • Fui tirando todo en un mismo archivo porque primero estaba viendo que funcionen las cosas, pero me gustaría que quedara un poco más ordenado en diferentes archivos en lugar de como está ahora.

  • Agregar la función que setea las traducciones por default en español.

  • Publicar una versión y cambiar en pilas-bloques-app para que use eso en lugar de local. De paso estaría bueno (pero puede ser pateado a otro issue), actualizar el README sobre su uso -> hay que basarse en el README de proceds, pero hay varias cosas que ya no están asi que hay que revisarlo.

src/index.js Outdated
Comment on lines 36 to 45
const disableContextMenuOptions = (Blockly) => {
const helpOption = Blockly.ContextMenuRegistry.registry.getItem('blockHelp')
const duplicateOption = Blockly.ContextMenuRegistry.registry.getItem('blockDuplicate')
const disableOption = Blockly.ContextMenuRegistry.registry.getItem('blockDisable')

disableOption.preconditionFn = () => 'disable'
duplicateOption.preconditionFn = () => 'disable'
helpOption.preconditionFn = () => 'disabled'

}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Esto lo agregué porque el menú que aparece al hacerle click derecho a los bloques, en producción hoy tiene desactivadas algunas opciones, quizás esto tendría que estar en pilas-bloques-app y no acá, es para revisar.

src/index.js Outdated
Comment on lines 55 to 58
const findLegalName = (name, block, index, workspace) => {
const newName = index === 0 ? name : (name + index)
return allProcedureNames(workspace).includes(newName) ? findLegalName(name, block, index + 1, workspace) : newName;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eso soluciona que no te deje crear procedimientos con el mismo nombre, hay una función de Blockly que lo hace (findLegalName) pero no estaba andando asi que lo hice a mano. Las funciones auxiliares las dejé con un export porque son funciones que eventualmente tenemos que pasar a pilas-bloques-app (están en ember), asi que como son de procedimientos, pueden quedar acá me pareció y solo las importamos cuando llegue el momento.

src/index.js Outdated
@@ -118,7 +140,7 @@ const addParameter = (self, Blockly, argName) => {
16,
16,
Blockly.Msg.VARIABLES_SET_CREATE_GET.replace('%1', name),
() => createParameterCaller(self, self.arguments_[argsAmount])()
() => createParameterCaller(self, self.arguments_[argsAmount], Blockly)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

La razón por la que Blockly está siendo pasado en todos lados por parámetros, es porque si no se lo pasa, toma otra instancia de Blockly que no tiene el main workspace, los bloques, etc, con los que queremos trabajar. Quizás hay una mejor forma de hacer esto, me quedó para revisar también porque es molesto que se esté pasando por todos lados.

src/index.js Outdated
Comment on lines 167 to 172
const varBlocks = self.workspace.getAllBlocks().filter(block.type === "variables_get" && block.$parent === self.id)
varBlocks.forEach(varBlock => {
var varField = varBlock.getField("VAR");
if (varField.getValue() === oldName) {
varField.setValue(newName);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Esto hay que revisarlo porque tampoco está cambiando el nombre del caller del parámetro cuando se cambia en el procedimiento

src/index.js Outdated
Comment on lines 223 to 238
const createCall = (self, Blockly) => {
console.log("create call")

const block = callbackFactory(self, createCallerXml(self), Blockly)

try {
const procedureBlock = self;

Blockly.Events.disabled_ = 1;
const posParent = procedureBlock.getRelativeToSurfaceXY();
const pos = block.getRelativeToSurfaceXY();
let width = procedureBlock.width;

block.moveBy(posParent.x - pos.x + width + 16, posParent.y - pos.y + 6);
} finally {
Blockly.Events.disabled_ = 0;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hay que probar con la versión de #1 (comment). Lo que probaría es que genere el mismo xml que se genera en producción/acá (la forma en que los crea a los bloques es armando un xml y de ahí lo crea). Si no crea el mismo, usaría esta forma, tenemos que mantener la misma estructura de xml para que funcionen las soluciones guardadas.

@dlopezalvas dlopezalvas marked this pull request as ready for review November 26, 2024 20:22
@@ -9,42 +9,39 @@
"lint": "blockly-scripts lint",
"predeploy": "blockly-scripts predeploy",
"start": "blockly-scripts start",
"test": "blockly-scripts test"
"test": "blockly-scripts test",
"prepublishOnly": "npm run build"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Jest necesita la versión compilada, asi que antes de cada publish tiene que hacerse un build, con este comando no es necesario tener que hacerlo manualmente, lo corre solo cuando se hace el npm publish.

Copy link
Contributor

@rgonzalezt rgonzalezt left a comment

Choose a reason for hiding this comment

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

🚀 🐐
la cabra que va al espacio deauuu

@dlopezalvas dlopezalvas merged commit 6acc9e6 into main Nov 28, 2024
@dlopezalvas dlopezalvas deleted the proceds-blockly-2 branch November 28, 2024 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants