Skip to content
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

Should onscreenschange provide useful data? #28

Open
tomayac opened this issue Sep 7, 2020 · 6 comments
Open

Should onscreenschange provide useful data? #28

tomayac opened this issue Sep 7, 2020 · 6 comments
Assignees
Labels
enhancement New feature or request question Further information is requested

Comments

@tomayac
Copy link
Contributor

tomayac commented Sep 7, 2020

Right now, a screenschange event doesn't convey the updated data, so you end up having to do this:

window.addEventListener('screenschange', async (e) => {
  console.log('I am there, but mostly useless', e);
  const details = await window.getScreens();
});

Ideally the new screen constellation would be somewhere contained in e.

@tomayac tomayac changed the title Should oscreenschange provide useful data? Should onscreenschange provide useful data? Sep 7, 2020
@tomayac
Copy link
Contributor Author

tomayac commented Sep 14, 2020

Right now, a screenschange event doesn't convey the updated data

The reason is probably that the window-placement permission might not be granted, but the event can fire nevertheless in the current implementation in Chrome. Maybe if the event only fired when the permission has been granted it could provide useful data to begin with.

@michaelwasserman
Copy link
Member

Thanks for this good feedback! In #30 I suggested adding data to the events (e.g. event.isMultiScreen and event.screens, as permitted) @jakearchibald had this solid advice (along with other API shape feedback that might yield broader outcomes resolving this issue):

Right now, the problem is that the event fires, but the information isn't available. However, I don't think the solution is to add that information to the event. In fact, the TAG recommends against it.

I think the information should be available synchronously once permission is granted.

I would only subclass the event if you want to provide information beyond current state. For example, it might be appropriate for the event to have information around what changed vs the previous state, but I don't think that's urgent.

@michaelwasserman michaelwasserman self-assigned this Sep 16, 2020
@michaelwasserman
Copy link
Member

Given the TAG recommendation against adding info to events, and the new API shape's (1) use of live objects to make updated information available when events are fired and (2) greater granularity of events (Screen[Advanced].onchange, Screens.onscreenschange, and Screens.oncurrentscreenchange), I doubt we will add info to the event objects themselves.

Clients can target more specific event handlers now, cache values of attributes they care to observe, and compare cached values against updated data in respective event handlers. We're definitely open to other feedback about the ergonomics here, so please let us know if this could be improved!

@michaelwasserman
Copy link
Member

Per offline discussion, @tomayac thinks we should still consider this.

"Maybe the approach here would be something like an observer (similar to MutationObserver)? On the one hand you need to observe changes to existing things, like the resolution of an already connected screen increasing, but on the other hand you also need to deal with new things entering, like a new screen being connected. I think the existing event model deals well with the former, but is less suitable for the latter. Was an observer pattern discussed in this context?"

I asked what info he'd expect the events to carry and their use cases; maybe Ids of screens added/removed for Screens.screenschange? Dictionaries paralleling the pre-change Screen data or a list with names of changed properties for Screens.currentscreenchange and Screen.onchange? He said:

"The more I think about it the more I am inclined to see the need for something that gives me the old state vs. the new state (maybe like a MutationRecord), so I can "calculate" the "diff" myself. As simple as a capture of getScreens() before and after the change. With an observer pattern the developer could specify what changes they are interested in, like ignore color depth changes but be informed of resolution changes. "

I said "IIUC, [that] can be polyfilled by caching, and might not actually help much. We can definitely add event info later, as needed, to address more concrete requests with use cases." He replied:

"+1 to tying this to concrete requests. ... we may want to hear from folks who arrange, say, multiple console windows or IDE windows and have experience with this sort of thing."

@Garbee
Copy link

Garbee commented Sep 4, 2021

An observer pattern is actually very compelling here. As one action may cause multiple things to happen in a series. For example, unplugging a monitor triggers:

  1. Monitor removed from an array of 3
  2. Monitors that are left could have their resolutions resized based on previous user configuration.
  3. Locations of monitors may move based on previous configuration

These could get batched by the OS or fired off each as their own events. It may be better for an observer event method to stream these through rather than trying to auto-batch them together. Points 2 and 3 could even each trigger their own series of events that may not be completely uniform across OSes and situations.

I don't have any concrete data on all this, just speaking from observations of having worked on Windows with multi-monitor stuff before and my Mac experience with docking to other monitors. Might be worth doing a quick investigation on how the main OSes handle this stuff concretely, then determining if they do batch events internally or if they fire off each thing as a specific occurrence.

@michaelwasserman michaelwasserman added enhancement New feature or request question Further information is requested labels Oct 29, 2021
@michaelwasserman
Copy link
Member

This is an enhancement request that may merit future consideration, but there's lingering questions about the utility and correctness of providing details with screen change events.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants