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

Simplify the implementation of the message buffering #2123

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

0HyperCube
Copy link
Member

This is an attempt to slightly simplify the logic of the message buffering, whilst also fixing the debug message tree during buffering such that the message parents are properly aligned.

Also improved was the functionality when attempting to run another message when a buffer is open. Instead of this new message being processed first, it is processed last.

sample message tree

Copy link

Found Clippy warnings

Clippy Warnings/Errors

warning: useless conversion to the same type: `messages::portfolio::document::document_message::DocumentMessage`
   --> editor/src/dispatcher.rs:134:7
    |
134 |                         DocumentMessage::UpdateClickTargets { click_targets }.into(),
    |                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider removing `.into()`: `DocumentMessage::UpdateClickTargets { click_targets }`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#useless_conversion
    = note: `-W clippy::useless-conversion` implied by `-W clippy::all`
    = help: to override `-W clippy::all` add `#[allow(clippy::useless_conversion)]`

warning: useless conversion to the same type: `messages::portfolio::document::document_message::DocumentMessage`
   --> editor/src/dispatcher.rs:135:7
    |
135 |                         DocumentMessage::UpdateClipTargets { clip_targets }.into(),
    |                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider removing `.into()`: `DocumentMessage::UpdateClipTargets { clip_targets }`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#useless_conversion

warning: `graphite-editor` (lib) generated 2 warnings (run `cargo clippy --fix --lib -p graphite-editor` to apply 2 suggestions)
warning: `graphite-editor` (lib) generated 2 warnings (2 duplicates)
warning: `graphite-editor` (lib test) generated 2 warnings (2 duplicates)
    Finished `dev` profile [optimized + debuginfo] target(s) in 4m 43s

    Finished `dev` profile [unoptimized + debuginfo] target(s) in 41.23s

Copy link
Collaborator

@adamgerhant adamgerhant left a comment

Choose a reason for hiding this comment

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

This looks good, I tested it with offset transforms and everything still works.

In general I'm still a bit uncertain if the message buffering is the correct solution to "async" messages that can only be run once a "promise" is resolved. There essentially needs to be a way for the message to be queued and executed once some data is available after some number of renders. Currently this is necessary for layers to await until their transform has been computed and returned, but in the future a more flexible system might be necessary where for instance a message can be queued for a certain number of milliseconds or until some state changes in the network, which may take multiple renders

if let Some(buffered_queue) = self.buffered_queue.take() {
self.message_queues.extend(buffered_queue);
self.message_queues = buffered_queue;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason why the buffered queue replaces the current message queues? I think it could be possible for there to still be messages in self.message_queues, which would be lost here.

Copy link
Member Author

Choose a reason for hiding this comment

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

The current message queues will always be empty (except for the end buffer event), since they are cleared by the start buffer event and all other messages are added directly to the buffer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants