Inconsistent state after barplot errors

[fedarko]

This is a small weird problem that nonetheless makes the UI look weird:

The problem is caused by setting the barplots to something invalid (e.g. trying to use continuous coloring with a categorical field, as shown in the GIF above), and then -- rather than fixing the error -- going and doing something else (e.g. changing the layout parameters).

What happens is the barplot state hasn't been reset to something that at least works, so -- although the canvas is changed -- I think what happens is stuff is bailed out of before draw() is called. This results in the weird scenario where the tree remains unchanged until the user does something like zooming or panning, as shown in the GIF above.

Some ways we could fix this:

• Detect when an error is thrown when trying to draw barplots (and, alternatively, when barplots are drawn successfully), and update an Empress (?) class-level boolean variable named barplotsBroken or something like that. Then, only call Empress.drawBarplots() from Empress.reLayout() if barplotsBroken isn't true. (Pressing the barplot panel Update button calls Empress.drawBarplots() directly, which gives us the chance to un-break the barplots.)

• Instead of throwing an error, immediately modify the barplot UI to automatically "fix" any erroneous settings and then use that -- e.g. uncheck the continuous values box, as is done in #484. (I think this would be difficult to implement smoothly for barplots, since there are more ways these can cause errors and messing with the UI automatically here might get confusing/annoying for users...)

I think the first option would be best, and shouldn't be too difficult to implement.

[ElDeveloper]

A broader solution would be to make redrawing events limited in their scope of what they can redraw, right? So if we are re-doing layout we shouldn't recompute the barplot data, unless the "update barplot" button is pressed. Any thoughts?

On Jun 11, 2021, at 7:59 PM, Marcus Fedarko @.***> wrote:

This is a small weird problem that nonetheless makes the UI look weird:

The problem is caused by setting the barplots to something invalid (e.g. trying to use continuous coloring with a categorical field, as shown in the GIF above), and then -- rather than fixing the error -- going and doing something else (e.g. changing the layout parameters).

What happens is the barplot state hasn't been reset to something that at least works, so -- although the canvas is changed -- I think what happens is stuff is bailed out of before draw() is called. This results in the weird scenario where the tree remains unchanged until the user does something like zooming or panning, as shown in the GIF above.

Some ways we could fix this:

• Detect when an error is thrown when trying to draw barplots (and, alternatively, when barplots are drawn successfully), and update an Empress (?) class-level boolean variable named barplotsBroken or something like that. Then, only call Empress.drawBarplots() from Empress.reLayout() if barplotsBroken isn't true. (Pressing the barplot panel Update button calls Empress.drawBarplots() directly, which gives us the chance to un-break the barplots.)

• Instead of throwing an error, immediately modify the barplot UI to automatically "fix" any erroneous settings and then use that -- e.g. uncheck the continuous values box, as is done in #484. (I think this would be difficult to implement smoothly for barplots, since there are more ways these can cause errors and messing with the UI automatically here might get confusing/annoying for users...)

I think the first option would be best, and shouldn't be too difficult to implement.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.

[fedarko]

I agree that that'd be ideal. There are some complications in practice due to how the code is structured, though: with the example of layouts and barplots, in order for us to re-draw the tree in a new layout, we need to also re-draw the barplots (since we can't use the same barplot coordinates for the circular and rectangular layouts). And in order to re-draw the barplots, we need to look at the current barplot state: and where things start to get messy is that adjusting the UI of the barplot layers (e.g. adding a new layer, changing a dropdown, etc) adjusts the state of the corresponding BarplotPanel / BarplotLayer objects. So the state of the barplot objects can and will get out of sync from what is actually shown in the display.

One way around this (and, I guess, option number 3 of solving this issue) is adjusting the barplot stuff so that the state is cached when Update is pressed (so that intermediate changes to the barplot UI don't take effect)... but I'm not sure how easy that'd be.

(Option number 4 would be just ditching the barplot update button entirely, but since drawing barplots for massive trees can take a while I'd prefer to avoid that for the time being...)