Skip to content

Transformed event-loop callback to FnMut to allow mutable values#2667

Merged
lucasfernog merged 3 commits into
tauri-apps:nextfrom
legion-labs:callback
Sep 27, 2021
Merged

Transformed event-loop callback to FnMut to allow mutable values#2667
lucasfernog merged 3 commits into
tauri-apps:nextfrom
legion-labs:callback

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Sep 27, 2021

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Docs
  • New Binding Issue #___
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes. Issue #___
  • No

The PR fulfills these requirements:

  • When resolving a specific issue, it's referenced in the PR's title (e.g. fix: #xxx[,#xxx], where "xxx" is the issue number)
  • A change file is added if any packages will require a version bump due to this PR per the instructions in the readme.

Other information:

Hi,

This PR fixes a limitation in Tauri's current callback design.

This Rust Playground example illustrates the problem.

Right now, Tauri's event loop callbacks are dyn Fn(RunEvent) + 'static which doesn't allow the caller to move-in mutable values. As this callback is typically provided for callers to be able to update their own event/game loop, and as these update methods are usually marked &mut self, the callers have no choice but to wrap their values in a Rc<RefCell<T>>, and to call borrow_mut() for each event.

Aside from the additional code required, it forces heap-usage and causes some extra-work for each iteration (granted: this is not a lot of work, but still: there is probably no good reason to have that extra-work when a simple change can avoid it).

The proposed changes make it possible for callers to simple move the mutable value in the callback and call it directly.

Let me know what you think of this, and I'll be happy to discuss it of course!

P.S: Thanks for all the good work on Tauri: it is an awesome project.

@ghost ghost self-requested a review September 27, 2021 15:33
Copy link
Copy Markdown
Member

@lucasfernog lucasfernog left a comment

Choose a reason for hiding this comment

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

looks good, can you add a change file? there's some examples on the .changes folder

@ghost ghost self-requested a review as a code owner September 27, 2021 17:20
@ghost
Copy link
Copy Markdown
Author

ghost commented Sep 27, 2021

@lucasfernog Done.

I am not familiar with the .changes folder process, but I suspect I had to document the eventual API breaks in each of the affected components?

(I just realized that I had opened the documentation on this but forgot to read it and checked the box anyway in my PR: sorry about that).

Please let me know if I made a mistake.

@lucasfernog
Copy link
Copy Markdown
Member

The .changes folder is a covector thing, it allows us to define changes on each crate, which turns into an automated changelog. So yeah it should describe changes on all crates.

@ghost
Copy link
Copy Markdown
Author

ghost commented Sep 27, 2021

@lucasfernog Thanks for the explanations. That makes sense.

I just updated my last commit to include the missing crates.

@lucasfernog
Copy link
Copy Markdown
Member

Thanks, nice work :)

@Mehdish1
Copy link
Copy Markdown

Mehdish1 commented Sep 27, 2021 via email

@ghost
Copy link
Copy Markdown
Author

ghost commented Sep 27, 2021

So I just added the missing changes for Windows too.

I get remaining a test failure that I'm not sure I understand:

---- api::file::test::check_read_binary stdout ----
thread 'api::file::test::check_read_binary' panicked at 'assertion failed: `(left == right)`
  left: `[35, 33, 47, 98, 105, 110, 47, 98, 97, 115, 104, 10, 10, 101, 99, 104, 111, 32, 34, 72, 101, 108, 108, 111, 32, 116, 104, 101, 114, 101, 34]`,
 right: `[35, 33, 47, 98, 105, 110, 47, 98, 97, 115, 104, 13, 10, 13, 10, 101, 99, 104, 111, 32, 34, 72, 101, 108, 108, 111, 32, 116, 104, 101, 114, 101, 34]`', core\tauri\src\api\file.rs:79:7

Looking at the array, the difference seems to be about the expected binary to contain Windows line-endings (13, 10) whereas mine only contains UNIX line-endings (10).

Not sure if this is a problem and how to fix it.

@lucasfernog
Copy link
Copy Markdown
Member

Seems unrelated, let's ignore that one for now.

@ghost
Copy link
Copy Markdown
Author

ghost commented Sep 27, 2021

Aside from the timeout, which I'm not sure is related to these changes, everything seems to be passing!

@lucasfernog
Copy link
Copy Markdown
Member

Yeah the timeout is just GH action and Rust being slow :) thanks for the PR!

@lucasfernog lucasfernog merged commit bdbf905 into tauri-apps:next Sep 27, 2021
@ghost ghost deleted the callback branch September 27, 2021 19:45
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