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

`TouchBarButton` doesn't update when inside a `TouchBarGroup` #11761

Closed
lukechilds opened this Issue Jan 28, 2018 · 11 comments

Comments

Projects
None yet
3 participants
@lukechilds

lukechilds commented Jan 28, 2018

  • Electron version: v1.7.11
  • Operating system: macOS

Expected behavior

Changing TouchBarButton properties should immediately update the button on the touch bar.

Actual behavior

Changing TouchBarButton properties has no effect when the TouchBarButton is inside a TouchBarGroup.

How to reproduce

const updatingButton = new TouchBarButton({'label': 'change'});
setInterval(() => updatingButton.label = '' + Math.random(), 1000);

// Button label updates every second
const touchBar = new TouchBar([updatingButton]);

// Button label never updates
const touchBar = new TouchBar([new TouchBarGroup({items: [updatingButton]})]);
@welcome

This comment has been minimized.

Show comment
Hide comment
@welcome

welcome bot Jan 28, 2018

👋 Thanks for opening your first issue here! If you're reporting a 🐞 bug, please make sure you include steps to reproduce it. We get a lot of issues on this repo, so please be patient and we will get back to you as soon as we can.

To help make it easier for us to investigate your issue, please follow the contributing guidelines.

welcome bot commented Jan 28, 2018

👋 Thanks for opening your first issue here! If you're reporting a 🐞 bug, please make sure you include steps to reproduce it. We get a lot of issues on this repo, so please be patient and we will get back to you as soon as we can.

To help make it easier for us to investigate your issue, please follow the contributing guidelines.

@lukechilds

This comment has been minimized.

Show comment
Hide comment
@lukechilds

lukechilds Jan 28, 2018

I can provide a clone-able gist for this later tonight.

lukechilds commented Jan 28, 2018

I can provide a clone-able gist for this later tonight.

@MarshallOfSound

This comment has been minimized.

Show comment
Hide comment
@MarshallOfSound

MarshallOfSound Jan 28, 2018

Member

Can you try on 1.8.2-beta.4? This sounds similar to an issue that has already been fixed

Member

MarshallOfSound commented Jan 28, 2018

Can you try on 1.8.2-beta.4? This sounds similar to an issue that has already been fixed

@MarshallOfSound

This comment has been minimized.

Show comment
Hide comment
@MarshallOfSound

MarshallOfSound Jan 28, 2018

Member

Refs: #9028

Looks like that fix only handles popovers not groups. Note to self, we should change _popovers to _parents to generify nested TB logic

Member

MarshallOfSound commented Jan 28, 2018

Refs: #9028

Looks like that fix only handles popovers not groups. Note to self, we should change _popovers to _parents to generify nested TB logic

@lukechilds

This comment has been minimized.

Show comment
Hide comment
@lukechilds

lukechilds Jan 29, 2018

Issue also occurs on TouchBarScrubber although interestingly sometimes the button appears to update when it's off canvas. When I drag the button back into view it stops updating again.

So are you aware of what's causing this then or do you still want a repro?

lukechilds commented Jan 29, 2018

Issue also occurs on TouchBarScrubber although interestingly sometimes the button appears to update when it's off canvas. When I drag the button back into view it stops updating again.

So are you aware of what's causing this then or do you still want a repro?

@lukechilds

This comment has been minimized.

Show comment
Hide comment
@lukechilds

lukechilds Jan 29, 2018

And to confirm, tried against v1.8.2-beta.4 and I'm getting the same issue.

lukechilds commented Jan 29, 2018

And to confirm, tried against v1.8.2-beta.4 and I'm getting the same issue.

MarshallOfSound added a commit that referenced this issue Feb 2, 2018

Fix child touch bar items not updating
Deep children of a TouchBar didn't cause the top level touch bar to update, now they do

Fixes #11761.
@MarshallOfSound

This comment has been minimized.

Show comment
Hide comment
@MarshallOfSound

MarshallOfSound Feb 2, 2018

Member

TouchBarScrubber is a bit weird in the sense that it doesn't have a child touch bar, rather a set of "items". You can make the scrubber update two ways.

// Option 1
scrubber.items = scrubber.items;

// Option 2
scrubber.emit('change', scrubber);

Edit: PR is up to fix groups though 👍

Member

MarshallOfSound commented Feb 2, 2018

TouchBarScrubber is a bit weird in the sense that it doesn't have a child touch bar, rather a set of "items". You can make the scrubber update two ways.

// Option 1
scrubber.items = scrubber.items;

// Option 2
scrubber.emit('change', scrubber);

Edit: PR is up to fix groups though 👍

MarshallOfSound added a commit that referenced this issue Feb 2, 2018

Fix child touch bar items not updating
Deep children of a TouchBar didn't cause the top level touch bar to update, now they do

Fixes #11761.
@lukechilds

This comment has been minimized.

Show comment
Hide comment
@lukechilds

lukechilds Feb 5, 2018

Would there be a way to hook into TouchBarScrubber items updates and then automatically do scrubber.emit('change', scrubber); inside Electron?

lukechilds commented Feb 5, 2018

Would there be a way to hook into TouchBarScrubber items updates and then automatically do scrubber.emit('change', scrubber); inside Electron?

@MarshallOfSound

This comment has been minimized.

Show comment
Hide comment
@MarshallOfSound

MarshallOfSound Feb 5, 2018

Member

@lukechilds We would effectively have to proxy all changes to the items array. (Pushing, slicing, popping and individual item prop updates). The API would have to dramatically change for this to be possible. That said this is something I'm open to because the TouchBar is marked as experimental so we can change the API if we want. It's just a lot of work so it's not on my immediate list of things to do especially when there is a relatively easy workaround 👍

Member

MarshallOfSound commented Feb 5, 2018

@lukechilds We would effectively have to proxy all changes to the items array. (Pushing, slicing, popping and individual item prop updates). The API would have to dramatically change for this to be possible. That said this is something I'm open to because the TouchBar is marked as experimental so we can change the API if we want. It's just a lot of work so it's not on my immediate list of things to do especially when there is a relatively easy workaround 👍

@lukechilds

This comment has been minimized.

Show comment
Hide comment
@lukechilds

lukechilds Feb 5, 2018

Hmmn, after trying the solution I'm not sure it's the best way forward.

I've tried manually setting scrubber.items and also emitting the 'change' event, however both of these reset the position of the scrubber.

Is this a limitation of the macOS touch bar API or just the Electron implementation?

It's quite confusing if the user has moved the scrubber to select something off the touch bar screen, then the scrubber randomly jumps back to the original position when an icon updates.

lukechilds commented Feb 5, 2018

Hmmn, after trying the solution I'm not sure it's the best way forward.

I've tried manually setting scrubber.items and also emitting the 'change' event, however both of these reset the position of the scrubber.

Is this a limitation of the macOS touch bar API or just the Electron implementation?

It's quite confusing if the user has moved the scrubber to select something off the touch bar screen, then the scrubber randomly jumps back to the original position when an icon updates.

@codebytere codebytere closed this in #11812 Feb 12, 2018

codebytere added a commit that referenced this issue Feb 12, 2018

Fix child touch bar items not updating (#11812)
* Fix child touch bar items not updating

Deep children of a TouchBar didn't cause the top level touch bar to update, now they do

Fixes #11761.

* Remove unused newValue property in TB setter
@lukechilds

This comment has been minimized.

Show comment
Hide comment
@lukechilds

lukechilds Feb 12, 2018

Does #11812 solve the issue of the touchbar scrubber jumping to the start position on update or is that a macOS limitation?

lukechilds commented Feb 12, 2018

Does #11812 solve the issue of the touchbar scrubber jumping to the start position on update or is that a macOS limitation?

sethlu added a commit to sethlu/electron that referenced this issue May 3, 2018

Fix child touch bar items not updating (#11812)
* Fix child touch bar items not updating

Deep children of a TouchBar didn't cause the top level touch bar to update, now they do

Fixes #11761.

* Remove unused newValue property in TB setter
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment