DDMAL / vida.js

Verovio Image Display Assistant
8 stars 4 forks source link

Notes regarding javascript compilation #7

Closed ahwitz closed 9 years ago

ahwitz commented 9 years ago

-make noLayout and other 1/0 parameters for Toolkit respond to boolean?

-is there an equivalent of bool hasData() to tell if I can run redoLayout?

-what exactly does redoLayout do?

lpugin commented 9 years ago

I think there was a problem jsonxx when using true/false. Or do you have something else in mind? On Nov 26, 2014 6:48 PM, "Andrew Horwitz" notifications@github.com wrote:

-make noLayout and other 1/0 parameters for Toolkit respond to boolean?

— Reply to this email directly or view it on GitHub https://github.com/DDMAL/vida.js/issues/7.

ahwitz commented 9 years ago

I wasn't going to bring these up to you until I had a large list and a full diagnosis. I'm using the 0.9.3 release for this example, but I'm compiling the most recent commit to get access to redoLayout as I type this up.

The issue is in https://github.com/DDMAL/vida.js/tree/booleanNoLayout: if you initiate it with horizontallyOriented: true, it loads as if noLayout = 0, then if you call $("#verovio-wrapper").data('vida').toggleOrientation(), it toggles horizontallyOriented (and thus noLayout, line 47) to false but still loads as if noLayout = 0.

If you initiate it with horizontallyOriented: false, it loads a div of the correct height but with no SVG components inside; toggling to true loads as if noLayout = 0, toggling again still loads if noLayout = 0.

ahwitz commented 9 years ago

(I had a bunch of stuff here but it's irrelevant now)

Overview of what happens:

0.9.3, noLayout = 0/1: as expected, paginated or one-system 0.9.3, noLayout = true/false: displays as noLayout = 0 when switching but with some glitches if you initiate it with noLayout = 1

most recent commit, noLayout = 0: as expected, paginates correctly most recent commit, noLayout = 1: glitch in how the command line version works where, unless pageWidth is explicitly set, everything displays as being condensed to pageWidth in width, even though the SVG says it's (in this case, 15900) pixels wide most recent commit, noLayout = true/false: same as 0.9.3

lpugin commented 9 years ago

OK, thanks. It reminds me of the issue I met with the true/false, which is why I switched to 0/1. Keep me posted.

On Wed, Nov 26, 2014 at 7:56 PM, Andrew Horwitz notifications@github.com wrote:

With the most recent commit, initiating it with true has the exact same results. Initiating it with false leads to a very heavily zoomed-in version of noLayout = 0, then has the same results with toggling.

Using 1/0 toggles between what I would expect for the paginated version of 0 and for 1 seems to be clipping the size of the SVG at the width of the parent div, but I think that's an issue on my end. Will update you as I progress.

— Reply to this email directly or view it on GitHub https://github.com/DDMAL/vida.js/issues/7#issuecomment-64694050.

ahwitz commented 9 years ago

I forgot you respond to things via email: I edited that post, it now reads:

Overview of what happens:

0.9.3, noLayout = 0/1: as expected, paginated or one-system 0.9.3, noLayout = true/false: displays as noLayout = 0 when switching but with some glitches if you initiate it with noLayout = 1

most recent commit, noLayout = 0: as expected, paginates correctly most recent commit, noLayout = 1: glitch in how the command line version works where, unless pageWidth is explicitly set, everything displays as being condensed to pageWidth in width, even though the SVG says it's (in this case, 15900) pixels wide most recent commit, noLayout = true/false: same as 0.9.3

lpugin commented 9 years ago

I can reproduce de problem in vida.js. Does it behaves the same in the command line, or is it just with the JS version?

ahwitz commented 9 years ago

Seems to be the same in the most recent commit of the command line; there's a bunch of weird stuff that happens with noLayout on. I'm pawing through the C++ to see if I can figure out why. If you beat me to it, let me know.

lpugin commented 9 years ago

I found it. It does some justification that needs to be disabled. I'll fix it

ahwitz commented 9 years ago

Heh. What line number was the problem, so I can see how it was working?

lpugin commented 9 years ago

It is happening on Page::LayOut (line 108)

lpugin commented 9 years ago

We need one member to say if we want justification to happen or not.

lpugin commented 9 years ago

Not sure where is the best place to put it...

ahwitz commented 9 years ago

Saw the commit and while I don't quite understand everything it does, I understand why it works. I had a few more related questions but they were all resolved with that fix.

lpugin commented 9 years ago

I just renamed Doc::LayOut to Doc::CastOff for clarifying this https://github.com/rism-ch/verovio/commit/e882b2aef88fbb18f49d5c7fd99b00d72fc09877 CastOff basically creates systems and pages starting from a single system with all the content. Page::LayOut calculates the position of all elements (horizontally and vertically) and justifies the page (currently only horizontally). Page::LayOut is triggered when the page is set in the View.

ahwitz commented 9 years ago

To let you know, either the ASM or Closure flag in Emscripten causes an issue with WebWorkers where the included Module variable appears to be undefined outside of the closure/ASM lines (beginning line 57 in the 0.9.4 release. Re-compiling without Closure seemed to fix it.

ahankinson commented 9 years ago

@ahwitz See: https://github.com/rism-ch/verovio/commit/bedd33b4e4b79b9889c153b10837433c4edeb9f1

ahwitz commented 9 years ago

Which is why I said it may be the ASM flag (still included in Line 29) may also be the cause and that the Module issue is also still present in the JS file released for 0.9.4.

ahwitz commented 9 years ago

I got it to work by running ./build.sh -l (L) with the Verovio emscripten build. The issue is that, with the regular build as a WebWorker, verovio-toolkit.js can't access the Module object in the verovio-wrapper-proxy code, and I'm not sure why.

I also have to comment out the window.addEventListener("unload") line, as I don't have access to the Window object in a WebWorker.

I'll upload a commit with this functional tomorrow.

ahwitz commented 9 years ago

Closing this as the issues raised here are all resolved.