jopeek / fvtt-simple-dice-roller

FVTT - Simple Dice Roller
8 stars 10 forks source link

Cannot return to previous scene control #3

Open kszczepanskidev opened 3 years ago

kszczepanskidev commented 3 years ago

Foundry VTT: 0.7.9 SDR: 1.1.2

When you open the SDR popup you cannot open tool that was selected previously.

Example steps to reproduce:

  1. Select Tile Controls
  2. Select SDR
  3. Try to open Tile Controls again, every other control works; closing the SDR by clicking again doesn't fix anything.
jopeek commented 3 years ago

It doesn't seem specific to the Tile Controls as I can also make this happen with others, but it is not consistent at all. No idea what's happening there. I can always get it to close by clicking other tools, though, so not a high priority for me to fix considering I can't reproduce it properly.

On Mon, Jan 4, 2021 at 10:14 AM Kamil Szczepański notifications@github.com wrote:

Foundry VTT: 0.7.9 SDR: 1.1.2

When you open the SDR popup you cannot open tool that was selected previously.

Example steps to reproduce:

  1. Select Tile Controls
  2. Select SDR
  3. Try to open Tile Controls again, every other control works; closing the SDR by clicking again doesn't fix anything.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/jopeek/fvtt-simple-dice-roller/issues/3, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJBV6VGNFWGLJRKR3Z2KVDSYIASBANCNFSM4VTLXHZQ .

-- Jan Ole Peek http://www.janolepeek.com/

kszczepanskidev commented 3 years ago

Yeah, I used Tile Controls as an example. For me it seems to be 100% consistent with select control X -> select SDR -> can't open tool x, but other buttons work.

jopeek commented 3 years ago

Yeah you're right, ok, will check some more.

On Mon, Jan 4, 2021 at 10:56 AM Kamil Szczepański notifications@github.com wrote:

Yeah, I used Tile Controls as an example. For me it seems to be 100% consistent with select control X -> select SDR -> can't open tool x, but other buttons work.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/jopeek/fvtt-simple-dice-roller/issues/3#issuecomment-754153046, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJBV6UQKSQYZQB7ZBAROXLSYIFOLANCNFSM4VTLXHZQ .

-- Jan Ole Peek http://www.janolepeek.com/

kszczepanskidev commented 3 years ago

I've fiddled with it a bit and seems that calling deactivate on the layer helps, but I'm not sure if there are no side effects.

canvas.stage.children.filter(layer => layer._active).forEach(layer => layer.deactivate());

jopeek commented 3 years ago

I think the problem really is that if you're on the Tiles icon, for example, and then you open SDR, and then click Tiles again, FVTT isn't aware that Tiles is NOT currently active, thinks that it still is, and thus does nothing. That's why when you click any other tool icon, it works fine, because FVTT thinks Tiles is active, you click Lighting, for example, and it works as intended.

On Mon, Jan 4, 2021 at 11:20 AM Kamil Szczepański notifications@github.com wrote:

I've fiddled with it a bit and seems that calling deactivate on the layer helps, but I'm not sure if there are no side effects.

canvas.stage.children.filter(layer => layer._active).forEach(layer => layer.deactivate());

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/jopeek/fvtt-simple-dice-roller/issues/3#issuecomment-754165402, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJBV6RX3FK7PRIJT5DLNK3SYIII7ANCNFSM4VTLXHZQ .

-- Jan Ole Peek http://www.janolepeek.com/

jopeek commented 3 years ago

I'm going to have to rewrite how SDR is added. It's not currently properly added as a new control but rather hacked into the DOM. That comes with limitations that cause that issue. I'm going to see if I can quickly do this, but if not, this may not be done by me anytime soon.

kszczepanskidev commented 3 years ago

Yeah, but I think that without hacking the DOM and using proper control items you can't get such nice list with buttons :)

If you want fast solution call this deactivate when opening SDR.

jopeek commented 3 years ago

Good call. Fixed in 1.1.3.

TheTural commented 3 years ago

I'm seeing some variant of this issue still on 1.1.3:

Happens 100% reliably on Firefox 84.0.2 and Chrome 87.0.4280.141 with Foundry 0.7.9 and D&D 5e 1.2.1 with no other modules enabled. (All those versions are the most current ones as of right now)

Attached video has other modules enabled but I confirmed it happens with all modules except SDR disabled by creating a brand new world and repeating the test.

https://user-images.githubusercontent.com/18756243/103855464-59556b80-5078-11eb-9935-6457f5ee3710.mp4

jopeek commented 3 years ago

I rolled back this "fix" because it made things demonstrably worse. I'd rather deal with not having the window close quite as easily than making all scene clicks wonky

kszczepanskidev commented 3 years ago

So in my module I've finally ended up with injecting code to the layers activated by the default buttons, which seems to be working far better for me. You can try and test it for yourself (and please test it before updating the module on live 😂 ):

function injectActivationResetForLayers() {
    canvas.stage.children.forEach(layer => {
        defaultActivate = layer.activate;
        layer.activate = function() {
            this.deactivate();
            return defaultActivate.apply(this, arguments);
        };
    });
}

The change to previous solution is that instead always deactivating current layer, we inject code that will deactivate the layer before trying to activate it (layers check if they are active before activating). Other solution could be to just overwrite property _active on the layer so it thinks it's not active, either way it seems to work better and only deselects the tokens when you return to the layer.