Implement dropShadow property which shows where widget will drop #1501

flackr · 2023-01-03T17:19:21Z

https://test.virtualtabletop.io/PR-1501/pr-test

While dragging a widget with the dropShadow property set, a clone of the widget is added to the current hover target to show where that widget will be dropped to visually.

Proposed wiki update:

Property	Default value	Description
dropShadow	false	If true, when a widget that matches the dropTarget is dragged over the holder, a gray shadow will appear where the widget will be when dropped. The shadow is a cloned widget with a property `dropShadowOwner`. When a shadow is created, the value will be the `playerName`. The clone will activate enterRoutine and leaveRoutine, so you may need to counteract that by using IF statements filtering out the clone looking for non-null values. The css of the shadow can be modified using `.dragging-shadow`.

While dragging a clone of the widget is added to target holders to show where that widget will be dropped to.

ArnoldSmith86 · 2023-01-03T17:20:23Z

PR-SERVER-BOT: You can play around with it here: https://test.virtualtabletop.io/PR-1501/pr-test (or any other room on that server)

After merging, a backup will be available at https://beta.virtualtabletop.io/editor/PR1501-pr-test.

96LawDawg · 2023-01-03T17:29:50Z

Doesn't work when dragging a pile handle.

96LawDawg · 2023-01-03T17:47:52Z

A couple of other things

Setting dropShadow: false on the holder seems like the more natural way to set the property, rather than on the widget. But maybe that is just me.
When you have a pile in a holder where the dropShadow will be, the pile number increments as soon as the dropShadow appears (because it is a widget getting counted in the pile). That is a minor change from current behavior where the pile number doesn't increment until the widget is dropped. Not sure this is a problem because it still works correctly with dropLimit. I thought it might not, but it does. So just pointing it out.
I think the shadow should be a lighter gray.
Can you explain how to change the css on the shadow? I tried something like this on the widget and it didn't work:

 "css": {
    ".shadow": {
      "filter": null,
      "background": "red"
    }
  },

96LawDawg · 2023-01-03T17:58:33Z

I added this to the cardDefaults thinking it might work:

"onPileCreation": {
      "dropShadow": true
    },

But that just makes the pile drag handle the target instead of the holder. So the pile drag handle turns gray.

flackr · 2023-01-03T18:10:12Z

I added this to the cardDefaults thinking it might work:
"onPileCreation": {
      "dropShadow": true
    },
But that just makes the pile drag handle the target instead of the holder. So the pile drag handle turns gray.

Yeah, this is because it currently does a shallow clone, so only the dragged widget (i.e. pile handle) is shown. This is an easy fix - and I can implement that other feature I've wanted - the recursive clone option.

flackr · 2023-01-03T19:42:56Z

Setting dropShadow: false on the holder seems like the more natural way to set the property, rather than on the widget. But maybe that is just me.

Yeah, I could see an argument for making this a property of the holder rather than the widget. It's slightly harder technically since I'll need to do an ancestor check but not that hard. I'd be curious to hear what others think.

When you have a pile in a holder where the dropShadow will be, the pile number increments as soon as the dropShadow appears (because it is a widget getting counted in the pile). That is a minor change from current behavior where the pile number doesn't increment until the widget is dropped. Not sure this is a problem because it still works correctly with dropLimit. I thought it might not, but it does. So just pointing it out.

Also something we could "fix" if we wanted - avoid counting the shadow widget in the pile count.

I think the shadow should be a lighter gray.

Can you explain how to change the css on the shadow? I tried something like this on the widget and it didn't work:
 "css": {
    ".shadow": {
      "filter": null,
      "background": "red"
    }
  },

I was able to style it via the global css, see the widget id css at https://test.virtualtabletop.io/PR-1501/shadow-style

Note that to disable the filter you have to set filter to 'none' or 'revert' to undo the filter being applied by the css stylesheet.

* Recursive clone to support complex structures. * Use id of shadow widget to not fail if something else deletes it. * Clone top child in presence of piles.

flackr · 2023-01-03T20:39:55Z

@96LawDawg I made a few changes around some of the things you brought up. Dragging a pile will now clone the top-most child in the pile rather than the pile handle. Also piles don't count the shadow widget anymore.

The clone code now also supports recursion and is shared with the clone function. Let me know what you think :-).

ArnoldSmith86 · 2023-01-03T20:42:22Z

Do you know about the existing clone function that's also used by the JSON editor command?

I didn't look at yours in detail yet but at a glance it seems like duplicated functionality.

robartsd · 2023-01-03T20:42:56Z

Do the shadow widgets get synced to state (all players see); if so, what happens with the clones if a client get disconnected in the middle of a dragging operation?

flackr · 2023-01-03T20:54:23Z

Do you know about the existing clone function that's also used by the JSON editor command?

I didn't look at yours in detail yet but at a glance it seems like duplicated functionality.

Yep, I'm aware, I've shared the code in the latest update of the PR.

Update: Oh, not the clone function, the editor command. I'll have to take a look.

flackr · 2023-01-03T20:57:10Z

Do the shadow widgets get synced to state (all players see);

Yes, I think this is the best way to make sure it behaves the same as dropping the widget and ensure that everything stays in sync.

if so, what happens with the clones if a client get disconnected in the middle of a dragging operation?

It stays. I'll look into how to detect drags still in process vs disconnected users and clean it up appropriately.

96LawDawg · 2023-01-03T21:53:39Z

Also piles don't count the shadow widget anymore.

Well, if you didn't feel fully welcomed to VTT yet, now you are. Because piles are hard. And your improvement is great and works as expected when a pile already exists. BUT, it doesn't work when first making a pile. Demo: Put 1 card into a holder that does piles. Then drag and hover over that holder. You get a pile indicator of 1 before there is a pile.

flackr · 2023-01-03T22:18:22Z

Well, if you didn't feel fully welcomed to VTT yet, now you are. Because piles are hard. And your improvement is great and works as expected when a pile already exists. BUT, it doesn't work when first making a pile. Demo: Put 1 card into a holder that does piles. Then drag and hover over that holder. You get a pile indicator of 1 before there is a pile.

Haha thanks!

I think what I really want is a property similar to isBeingRemoved which prevents piles from being formed - but I'm not sure if I can safely re-use that property. As a short term workaround I could probably set onPileCreation to something unique to prevent the shadow from being included in any piles.

flackr · 2023-01-03T22:24:04Z

And done! That was pretty simple and builds on the existing pile code rather than needing to modify it 😀

robartsd · 2023-01-03T22:31:53Z

It stays. I'll look into how to detect drags still in process vs disconnected users and clean it up appropriately.

We probably want the shadow widget to have a property that indicates which widget it is a shadow of. Not sure how it should be cleaned up. IIRC we have some code in timer.js that has another client adopt a running timer if the client that had been running the timer disconnects; if so, something similar should happen here.

flackr · 2023-01-03T22:45:37Z

It stays. I'll look into how to detect drags still in process vs disconnected users and clean it up appropriately.

We probably want the shadow widget to have a property that indicates which widget it is a shadow of. Not sure how it should be cleaned up. IIRC we have some code in timer.js that has another client adopt a running timer if the client that had been running the timer disconnects; if so, something similar should happen here.

Another option is we could just clean it up the next time someone drags the widget, since that would also be the point at which we clean up the dragging property right?

robartsd · 2023-01-03T22:54:35Z

Another option is we could just clean it up the next time someone drags the widget, since that would also be the point at which we clean up the dragging property right?

That's true. As long as the client that starts the new drag finds the old shadow copies rather than creating new ones, it should work fine.

I'm still not sure that I think the shadow elements should be widgets, but I understand that it makes some things easier to process them as widgets. A couple of years ago, we started down the process of making piles no longer be widgets, but eventually decided it wasn't worth it.

flackr · 2023-01-04T00:14:09Z

Another option is we could just clean it up the next time someone drags the widget, since that would also be the point at which we clean up the dragging property right?

That's true. As long as the client that starts the new drag finds the old shadow copies rather than creating new ones, it should work fine.

Done in latest patch.

I'm still not sure that I think the shadow elements should be widgets, but I understand that it makes some things easier to process them as widgets. A couple of years ago, we started down the process of making piles no longer be widgets, but eventually decided it wasn't worth it.

I know it's tempting not to create widgets for this, and I really tried to go this route initialy but there were going to be many issues that would be really hard to solve properly if it's not a widget. The state would be inconsistent if the widget isn't actually added to the holder during the drag. I.e. My client needs to produce a layout with the widget in the holder and if I have a different layout than you then it can lead to further inconsistencies. Not to mention there are many setups where the widget will run a routine to position new elements.

I initially tried putting the actual widget in the holder then it will break things like transitions #1423 and we'd still need to sync a drag copy of the widget for other people to see me dragging it anyways.

I think it's kind of nice how it works that you can see the same state and we can both position drag elements within holders in parallel.

robartsd · 2023-01-04T19:34:44Z

and we'd still need to sync a drag copy of the widget for other people to see me dragging it anyways.

All that is really needed for sync is for other clients to know that the widget is being dragged (which they already do) - each client could create the shadow element for widgets being dragged independently. Still I understand that using an actual widget makes a whole bunch of things about the shadow element easier to accomplish.

flackr · 2023-01-04T19:39:52Z

All that is really needed for sync is for other clients to know that the widget is being dragged (which they already do) - each client could create the shadow element for widgets being dragged independently. Still I understand that using an actual widget makes a whole bunch of things about the shadow element easier to accomplish.

That works for the simple cases, but if placing the shadow widget in the holder invokes routines you would invoke those routines on each client which would could result in strange unexpected results (e.g. if you had a counter it would be invoked by each player in the session).

My intention with just having a simple property is that we could still explore other ways of accomplishing the shadow while still supporting the high level developer intent that we should have a shadow element showing where the widget will go.

client/css/widgets/basicwidget.css

robartsd · 2023-01-04T20:25:47Z

That works for the simple cases, but if placing the shadow widget in the holder invokes routines you would invoke those routines on each client which would could result in strange unexpected results (e.g. if you had a counter it would be invoked by each player in the session).

If the shadow element were not a widget, it wouldn't trigger any routines. One concern I have is that by using shadow widgets, routines may be triggered that the game designer does not intend to be triggered until the widget is dropped.

If dropShadow is a holder property instead of a widget property (would not support a shadow outside holders, but I don't think that would change the visual effect unless the widget has partial transparency) the holder could create the shadow element as part of its own rendering. Not sure how that delta call should work though.

flackr · 2023-01-14T17:52:05Z

There are so many games out there that wouldn't benefit from this without file updater. It's fine but I don't see a reason yet why we shouldn't do it when the hand is just a default hand.

Yeah, it would be awesome to get this feature part of more games. +1 to trying to do this.

no parentChangeRoutine or changeRoutine on the card (or pile)

changeRoutines on the pile should be fine with the current PR because the shadow widget is not allowed to be part of piles (in order to avoid incorrect pile counts): https://github.com/ArnoldSmith86/virtualtabletop/pull/1501/files#diff-cda5cc45823227e11eaa23e3e718a50f300e3fa6183280043e48e4e55b78024dR2239 . That said I expect they're also extremely uncommon on piles anyways and it makes the code pattern more generic if I don't differentiate piles from other stuff.

…t on server-side.

flackr · 2023-01-25T22:46:43Z

I tested each of the public library games which was auto-modified by the file updater and as far as I can tell the added dropShadows does not affect the gameplay adversely in any of them. In most of them it is useful except for Rummy Tiles where the shadow isn't visible because it shows directly behind the widget anyways.

Can someone explain the remaining testcafe failure? How does the edit test know that after adding some widgets they must have a particular id? (i.e. in this case #w_3fseC1). I ran through the steps manually and didn't encounter any errors - the widgets were added the same as they were on the main branch.

robartsd · 2023-01-26T00:54:14Z

Can someone explain the remaining testcafe failure? How does the edit test know that after adding some widgets they must have a particular id? (i.e. in this case #w_3fseC1). I ran through the steps manually and didn't encounter any errors - the widgets were added the same as they were on the main branch.

The ID is generated randomly, but for TestCafe, all randomness is not so random (see prepareClient() in test-util.js) so the exact same sequence of values comes each time. The shadow widgets are getting assigned a random ID, so all subsequent random values are no longer the same as they were before these changes.

flackr · 2023-01-26T02:12:37Z

The ID is generated randomly, but for TestCafe, all randomness is not so random (see prepareClient() in test-util.js) so the exact same sequence of values comes each time. The shadow widgets are getting assigned a random ID, so all subsequent random values are no longer the same as they were before these changes.

Yeah that makes sense. Do you have a way to figure out what the ids have become? Every time I try to run the tests locally it times out waiting for the browser and the test failure on the run here doesn't say anything about what ids were created. I suppose I could add some logging.

$ npm run testcafe-firefox-headless-all

> [email protected] testcafe-firefox-headless-all
> testcafe firefox:headless tests/testcafe

Browserslist: caniuse-lite is outdated. Please run:
  npx browserslist@latest --update-db
  Why you should do it regularly: https://github.com/browserslist/browserslist#browsers-data-updating
ERROR Cannot establish one or more browser connections.
1 of 1 browser connections have not been established:
- firefox:headless

Hints:
- Increase the value of the "browserInitTimeout" option if it is too low (currently: 2 minutes for local browsers and 6 minutes for remote browsers). This option determines how long TestCafe waits for browsers to be ready.
- The error can also be caused by network issues or remote device failure. Make sure that your network connection is stable and you can reach the remote device.

Type "testcafe -h" for help.

ArnoldSmith86 · 2023-01-26T06:26:06Z

It's possible to use the non-random random function in a proper browser and do all the clicks.

ArnoldSmith86 · 2023-01-26T21:14:30Z

Does TestCafé also fail with Chrome/Chromium? You should also be able to use it on gitpod.io: https://gitpod.io/#https://github.com/flackr/virtualtabletop/tree/drop-shadow , https://github.com/ArnoldSmith86/virtualtabletop/wiki/TestCaf%C3%A9#running-tests-in-gitpod .

robartsd · 2023-01-26T22:27:26Z

Does TestCafé also fail with Chrome/Chromium? You should also be able to use it on gitpod.io: https://gitpod.io/#https://github.com/flackr/virtualtabletop/tree/drop-shadow , https://github.com/ArnoldSmith86/virtualtabletop/wiki/TestCaf%C3%A9#running-tests-in-gitpod .

~~Not currently working correctly in gitpod~~

Instructions needed to be updated after #1362

I was able to install noVNC with source tests/testcafe/gitpod-vnc/install.sh then run npm run testcafe-firefox -- --debug-on-fail firefox tests/testcafe/editor.js to get the test to stop at the failing line leaving the window open for inspection.

flackr · 2023-01-27T15:42:57Z

Thanks @robartsd for the test update! I filed a separate issue #1565 for issues around running testcafe. I'll try your instructions out on my system.

ArnoldSmith86

Code looks fine to me but it's complex enough that it'll probably break a thing or two. 🤷‍♂️

See if my two comments warrant a change and then merge.

server/fileupdater.mjs

Implement dropShadow property which shows where widget will drop

5133854

While dragging a clone of the widget is added to target holders to show where that widget will be dropped to.

flackr requested review from RaphaelAlvez and ArnoldSmith86 as code owners January 3, 2023 17:19

flackr added 2 commits January 3, 2023 15:26

Multiple changes to improve capability

bd84eba

* Recursive clone to support complex structures. * Use id of shadow widget to not fail if something else deletes it. * Clone top child in presence of piles.

Don't count shadow widget in pile counts.

c053cc3

Use randomized onPileCreation to prevent piles from forming.

53c9e29

Sync drop shadow widget property so widget is eventually cleaned up.

fc76145

robartsd reviewed Jan 4, 2023

View reviewed changes

client/css/widgets/basicwidget.css Show resolved Hide resolved

Review comments.

451fbe1

flackr added 8 commits January 15, 2023 22:32

Merge branch 'main' into drop-shadow

2b98078

Merge branch 'main' into drop-shadow

af0809f

Only try to create hoverTarget when the target changes.

74916f1

Track owner of drop shadow to make removing easier and more consisten…

5b91904

…t on server-side.

Define clonedFrom

a4007ef

Only try to position shadow widget if we have a hover target.

2364aff

Merge branch 'main' into drop-shadow

eb9df80

Update PL expectations due to fileupdater.

d0bee59

flackr requested a review from UltimateGeek as a code owner January 25, 2023 17:28

update create game in edit mode

795776b

robartsd mentioned this pull request Jan 27, 2023

testcafe times out waiting for browser. #1565

Open

ArnoldSmith86 approved these changes Feb 8, 2023

View reviewed changes

server/fileupdater.mjs Outdated Show resolved Hide resolved

server/fileupdater.mjs Outdated Show resolved Hide resolved

flackr mentioned this pull request Feb 9, 2023

Some transitions play when loading or widgets are added #1590

Closed

flackr added 7 commits February 8, 2023 20:04

Check faceTemplates, onEnter and onLeave as well for condition.

305fcef

Remove faceTemplates - AFAICT they dont set properties on widgets.

747acd8

Merge branch 'main' into drop-shadow

de67be4

Update hashes.

912bddd

Check faceTemplate properties.

41c3830

Merge branch 'main' into drop-shadow

57311e9

Merge branch 'main' into drop-shadow

8007b58

flackr merged commit 4a95460 into ArnoldSmith86:main Feb 10, 2023

rogerl50 mentioned this pull request Feb 13, 2023

CLONE offsetX and offsetY do not work #1612

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement dropShadow property which shows where widget will drop #1501

Implement dropShadow property which shows where widget will drop #1501

flackr commented Jan 3, 2023 •

edited by 96LawDawg

Loading

ArnoldSmith86 commented Jan 3, 2023

96LawDawg commented Jan 3, 2023

96LawDawg commented Jan 3, 2023

96LawDawg commented Jan 3, 2023

flackr commented Jan 3, 2023

flackr commented Jan 3, 2023

flackr commented Jan 3, 2023

ArnoldSmith86 commented Jan 3, 2023

robartsd commented Jan 3, 2023 •

edited

Loading

flackr commented Jan 3, 2023 •

edited

Loading

flackr commented Jan 3, 2023

96LawDawg commented Jan 3, 2023

flackr commented Jan 3, 2023

flackr commented Jan 3, 2023

robartsd commented Jan 3, 2023

flackr commented Jan 3, 2023

robartsd commented Jan 3, 2023 •

edited

Loading

flackr commented Jan 4, 2023

robartsd commented Jan 4, 2023

flackr commented Jan 4, 2023 •

edited

Loading

robartsd commented Jan 4, 2023

flackr commented Jan 14, 2023

flackr commented Jan 25, 2023

robartsd commented Jan 26, 2023

flackr commented Jan 26, 2023

ArnoldSmith86 commented Jan 26, 2023

ArnoldSmith86 commented Jan 26, 2023

robartsd commented Jan 26, 2023 •

edited

Loading

flackr commented Jan 27, 2023

ArnoldSmith86 left a comment

Implement dropShadow property which shows where widget will drop #1501

Implement dropShadow property which shows where widget will drop #1501

Conversation

flackr commented Jan 3, 2023 • edited by 96LawDawg Loading

ArnoldSmith86 commented Jan 3, 2023

96LawDawg commented Jan 3, 2023

96LawDawg commented Jan 3, 2023

96LawDawg commented Jan 3, 2023

flackr commented Jan 3, 2023

flackr commented Jan 3, 2023

flackr commented Jan 3, 2023

ArnoldSmith86 commented Jan 3, 2023

robartsd commented Jan 3, 2023 • edited Loading

flackr commented Jan 3, 2023 • edited Loading

flackr commented Jan 3, 2023

96LawDawg commented Jan 3, 2023

flackr commented Jan 3, 2023

flackr commented Jan 3, 2023

robartsd commented Jan 3, 2023

flackr commented Jan 3, 2023

robartsd commented Jan 3, 2023 • edited Loading

flackr commented Jan 4, 2023

robartsd commented Jan 4, 2023

flackr commented Jan 4, 2023 • edited Loading

robartsd commented Jan 4, 2023

flackr commented Jan 14, 2023

flackr commented Jan 25, 2023

robartsd commented Jan 26, 2023

flackr commented Jan 26, 2023

ArnoldSmith86 commented Jan 26, 2023

ArnoldSmith86 commented Jan 26, 2023

robartsd commented Jan 26, 2023 • edited Loading

flackr commented Jan 27, 2023

ArnoldSmith86 left a comment

Choose a reason for hiding this comment

flackr commented Jan 3, 2023 •

edited by 96LawDawg

Loading

robartsd commented Jan 3, 2023 •

edited

Loading

flackr commented Jan 3, 2023 •

edited

Loading

robartsd commented Jan 3, 2023 •

edited

Loading

flackr commented Jan 4, 2023 •

edited

Loading

robartsd commented Jan 26, 2023 •

edited

Loading