Skip to content

Scheduler: Selection logic #6098

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

Merged
merged 32 commits into from
May 21, 2025
Merged

Scheduler: Selection logic #6098

merged 32 commits into from
May 21, 2025

Conversation

stsrki
Copy link
Collaborator

@stsrki stsrki commented May 12, 2025

An attempt to implement slot selection logic for creating new items-

@stsrki stsrki requested a review from tesar-tech May 12, 2025 10:31
@stsrki
Copy link
Collaborator Author

stsrki commented May 12, 2025

The SchedulerSelectingTransaction might not be needed at all. For now I'm keeping it.

@stsrki
Copy link
Collaborator Author

stsrki commented May 15, 2025

@tesar-tech Please review.

The API is now

[Parameter] public SchedulerSlotSelectionMode SlotSelectionMode { get; set; }
[Parameter] public EventCallback<SchedulerSlotsSelectedEventArgs> SlotsSelected { get; set; }

The enum SchedulerSlotSelectionMode is used so that we have felicibility to add another method of selection in the future. Like a Keyboard.

The SlotsSelected callback is added so that the user can know what to do in case they do not use internal editing.

@stsrki stsrki added this to the 1.8 milestone May 16, 2025
Copy link
Contributor

@tesar-tech tesar-tech left a comment

Choose a reason for hiding this comment

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

It works. The only ui issue I saw was:

  • Click and drag to create an event
  • cancel even
  • the clicked slot now has the "hover" bg (see north-west cell from the cursor)
    image

Overall it builds upon the issues that Scheduler brought.

For example views overall should be solved differently - primarily using generic and just switching the implementation for SelectedView. Would save a lot of code when extending the functionality like in this pr. This is a pattern I see throughout the whole implementation, so I guess that's not something to be corrected and I will just go with it.

I will look into the pr one more time (and check the js for example). After this is resolved.

@stsrki
Copy link
Collaborator Author

stsrki commented May 20, 2025

It works. The only ui issue I saw was:

  • Click and drag to create an event
  • cancel even
  • the clicked slot now has the "hover" bg (see north-west cell from the cursor)
    image

Overall it builds upon the issues that Scheduler brought.

For example views overall should be solved differently - primarily using generic and just switching the implementation for SelectedView. Would save a lot of code when extending the functionality like in this pr. This is a pattern I see throughout the whole implementation, so I guess that's not something to be corrected and I will just go with it.

I will look into the pr one more time (and check the js for example). After this is resolved.

I cannot reproduce this.

@stsrki stsrki requested a review from tesar-tech May 20, 2025 07:50
@tesar-tech
Copy link
Contributor

I cannot reproduce this.

Me neither. Probably fixed along with the @key implementation.


But now it seems to me some "perf" issue is happening. When I click a slot the dialog appears smoothly, but when I drag it, the dialog is bit laggy.

Screencast.From.2025-05-21.09-01-52.webm

It is just a bit slower, but noticeable and constant. Which could mean something is off under the hood.

@stsrki
Copy link
Collaborator Author

stsrki commented May 21, 2025

I cannot reproduce this.

Me neither. Probably fixed along with the @key implementation.

But now it seems to me some "perf" issue is happening. When I click a slot the dialog appears smoothly, but when I drag it, the dialog is bit laggy.

Screencast.From.2025-05-21.09-01-52.webm
It is just a bit slower, but noticeable and constant. Which could mean something is off under the hood.

I have added a mouse event throttle that might need adjustments. For now, it is set to 150ms.

@tesar-tech
Copy link
Contributor

tesar-tech commented May 21, 2025

I have added a mouse event throttle that might need adjustments. For now, it is set to 150ms.

I see, that might be it. The lag is also noticeable when you drag. What's the reason behind the throttle?


I checked the js. The reason why we need it is the right time call for canceling the selection (mainly the mouseup on the document)? Everything else is handled in .net. Is that right?

I don't like how the selectionEnded goes from both sides. Maybe considering the removal of MouseUp events and calling the necessary methods from selectionEnded, not the other way around?

  • I don't see a case where selectionEnded call from c# would not end up in.
  if (!instance || !instance.mouseUpHandler) {
        return;
    }
  • There are cases where only the js selectionEnded is called, but MouseUp is not, leaving the ui left behind:
    (when you drag inside and release outside of the scheduler)
Screencast.From.2025-05-21.09-40-00.webm

The js selectionEnded is called here from the js handler. Calling the c# here would solve the "bi-directionality" and also the ui left behind.


Edit: It's not only when you mouseUp outside of the Scheduler, but outside of the current column.

@stsrki
Copy link
Collaborator Author

stsrki commented May 21, 2025

I see, that might be it. The lag is also noticeable when you drag. What's the reason behind the throttle?

Mousemove events are called on each per-pixel move, which is too much and can lead to too many re-renderings. So I have throtled them to be executed less often.


I checked the js. The reason why we need it is the right time call for canceling the selection (mainly the mouseup on the document)? Everything else is handled in .net. Is that right?

The reason is that mouseup only work on element that has the event registered. So if we have the event on a slot element, and we release mouse anywhere else, it will not be registered. Leading to an invalid selection state until we click mouse again.

Also, the problem you described in the video should be solved with the JS. I will look into it.

@stsrki
Copy link
Collaborator Author

stsrki commented May 21, 2025

I cannot reproduce the issue in the video.

In any case I have slightly changed logic in selectionStarted.

@tesar-tech
Copy link
Contributor

Mousemove events are called on each per-pixel move, which is too much and can lead to too many re-renderings. So I have throtled them to be executed less often.

Make sense, but the 150 jumps dont leave a good impression.

The reason is that mouseup only work on element that has the event registered. So if we have the event on a slot element, and we release mouse anywhere else, it will not be registered. Leading to an invalid selection state until we click mouse again.

I might be too late, but what about:

  • keep slots subscribed to mouse down.
  • mouse is down - the column calls js to register mouseenter mouseleave on all the slots. Also the document mouseup.
  • mouseenter calls c# and it will do the border, together with the mouseleave. It will also mark the slots (which is done).
  • mouseup - unsubscribe all the events, calls dotnet to finish the transaction.

No need for throttle, universal response for mouseup, strictly defined direction of the events.

@stsrki
Copy link
Collaborator Author

stsrki commented May 21, 2025

I might be too late, but what about:

  • keep slots subscribed to mouse down.
  • mouse is down - the column calls js to register mouseenter mouseleave on all the slots. Also the document mouseup.
  • mouseenter calls c# and it will do the border, together with the mouseleave. It will also mark the slots (which is done).
  • mouseup - unsubscribe all the events, calls dotnet to finish the transaction.

No need for throttle, universal response for mouseup, strictly defined direction of the events.

I tried that. The problem then was the synchronization between blazor and js. I still need to render selection element. So if go all-in with JS then I would have to provide it with the CSS classnames that are used by the currently running CSS provider. It worked, but it was more complex in the end.

It was easier to go mostly with C#.

@tesar-tech
Copy link
Contributor

I tried that. The problem then was the synchronization between blazor and js. I still need to render selection element. So if go all-in with JS then I would have to provide it with the CSS classnames that are used by the currently running CSS provider. It worked, but it was more complex in the end.

Styling of the selection element would be through c# still, just the event "messaging" would originate in js mostly. The main idea here is the mouseenter/leave instead of mousemove..


If you are ok with the 150ms "lag" I have no any other objection to solve.

@stsrki
Copy link
Collaborator Author

stsrki commented May 21, 2025

If you are ok with the 150ms "lag" I have no any other objection to solve.

Optimization can always be done later. As long as the API is stable, I'm OK.

@stsrki stsrki merged commit a04cf3e into dev-scheduler May 21, 2025
1 of 2 checks passed
@stsrki stsrki deleted the dev-scheduler-selection branch May 21, 2025 15:36
@github-actions github-actions bot locked and limited conversation to collaborators May 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants