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

reset idle timeout when pushing a new compositor layer #5660

Closed

Conversation

pascalkuthe
Copy link
Member

Fixes #4956

As suggested in #4956 (comment) the right fix is to
reset the idle timer when pushing new layers into the compositor.

@pascalkuthe pascalkuthe added C-bug Category: This is a bug E-easy Call for participation: Experience needed to fix: Easy / not much A-helix-term Area: Helix term improvements S-waiting-on-review Status: Awaiting review from a maintainer. labels Jan 24, 2023
@the-mikedavis
Copy link
Member

I see what you were saying in #5115 with the callsites all adding editor args and that being a bit noisy. I don't see a better way to give Compositor::push access to the idle timer though so I think this looks good 👍

Copy link
Contributor

@Philipp-M Philipp-M left a comment

Choose a reason for hiding this comment

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

LGTM, fixes also when just opening the file-picker with a low idle-timeout (same issue as #4956)

@the-mikedavis the-mikedavis changed the title reset idle timeout when pushing a new composistor layer reset idle timeout when pushing a new compositor layer Jun 18, 2023
@Philipp-M
Copy link
Contributor

Is there anything blocking this? This seems relatively straight-forward.

@Philipp-M
Copy link
Contributor

Well apart from the fact, being completely out of sync with master. I have decided against fixing merge/rebase conflicts and replay the changes (as it seemed simpler than fixing the conflicts) for my own fork.

Here's a version that is has no merge conflicts with master: 97f310a

@pascalkuthe
Copy link
Member Author

pascalkuthe commented Jul 23, 2023

I was waiting for a decision from @archseer whether he wants to go forward with this. If he approves of the general idea I would rebase. In the long term this is not really the the right fix since I mostly want to rebase the kitchensinky idle timeout with a more granular event system. So it may not be worth to merge this over that.

@Philipp-M
Copy link
Contributor

Yeah absolutely agree with a finer-grained idle-timeout system, thought about that myself already.
But since this PR doesn't add up a lot of complexity and fixes an obvious bug, I think it makes sense to merge this in the mean time (can be refactored/rewritten later anyways).

@pascalkuthe
Copy link
Member Author

I already have a working event system locally (just haven't go around to finishing it) so its not too far away :)

I would be ok with rebasing and merging this it depends on what @archseer thinks

@the-mikedavis
Copy link
Member

@pascalkuthe do we need this anymore now that we have #8021?

@pascalkuthe
Copy link
Member Author

pascalkuthe commented Feb 10, 2024

Fixing this still needs some work to switch the picker away from the idle timeout but yeah let's close this. I don't think this is the right approach now thatwe have the Event system

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements C-bug Category: This is a bug E-easy Call for participation: Experience needed to fix: Easy / not much S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No syntax highlighting in preview on global search
3 participants