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

Allow mercury footer/header update based on content #4165

Merged
merged 4 commits into from
Sep 12, 2024

Conversation

obrusvit
Copy link
Contributor

@obrusvit obrusvit commented Sep 6, 2024

This PR modifies the mercury Frame so that it's able to hold a function pointer into a Footer updating function and Header updating function. This is useful so that the Footer or Header can react to the changes in the footer's content without making additional wrappers around Frame.
This functionality is applied to ShowShareWords, ConfirmFido and it also adds a footer page hint functionality to RemainingShares window which didn't make it in #4142 . Another candidate might be address_detail.rs

In addition, the PR removes some unused code from the Frame.

For future reference, other approaches were considered for the update functionality:

  • passing a reference of the content to the Footer which would be something like &dyn InternallySwipable. This is difficult to do because of lifetimes.
  • making a an update function in frame which would update the footer if the content was InternallySwipable. This would require "specialization" feature which is not stabilized
  • using a closure Fn instead of a function pointer fn. This is difficult because Frame must implement Clone and Fn is not Sized and we do not have a smart pointer wrapper.

@obrusvit obrusvit added the T3T1 Trezor Safe 5 label Sep 6, 2024
@obrusvit obrusvit self-assigned this Sep 6, 2024
@obrusvit obrusvit requested review from mmilata and removed request for prusnak September 6, 2024 14:51
@obrusvit
Copy link
Contributor Author

obrusvit commented Sep 6, 2024

The new footer in RemainingShares window:

  • one page only
    emu00000000

  • more pages
    emu_remaining_shares_0 emu_remaining_shares_1

cc: @Hannsek @lapohoda

Copy link

github-actions bot commented Sep 6, 2024

core UI changes device test click test persistence test
T2T1 Model T test(screens) main(screens) test(screens) main(screens) test(screens) main(screens)
T3B1 Safe 3 test(screens) main(screens) test(screens) main(screens) 2724
T3T1 test(screens) main(screens) test(screens) main(screens) test(screens) main(screens)
All main(screens)

(TR::instructions__swipe_up.into(), TR::recovery__more_shares_needed.into())
} else {
(TString::empty(), TString::empty())
};
Copy link
Member

Choose a reason for hiding this comment

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

Possibly unnecessary, PageHint already contains logic to display empty strings under single-page content (footer.rs:444).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you're right, addressed on rebase

@matejcik
Copy link
Contributor

maybe not in this PR but ... how about flipping the dependency relationship and making frame a member of whoever wants to be doing the footer updates?

@obrusvit
Copy link
Contributor Author

obrusvit commented Sep 11, 2024

maybe not in this PR but ... how about flipping the dependency relationship and making frame a member of whoever wants to be doing the footer updates?

@matejcik If I understand your suggestion correctly, that's what was happening before and it leads to creating more and more structs around the components and implementing Component for them with considerable code duplication.

struct SomeComplexScreen {
  frame: Frame<Something>,
  footer: Footer,
}

impl Component for SomeComplexScreen {
  fn event(...) {
    frame.footer.update_instruction(...);
    ...
  }
}

It seems to me cleaner to create those screens right away using regular components for the flow, now with the ability to change their content.

Frame::new(title, Something::new()).with_footer(...).register_footer_update_fn(some_fn);

I'm willing to discuss further.

@matejcik
Copy link
Contributor

@matejcik If I understand your suggestion correctly, that's what was happening before and it leads to creating more and more structs around the components and implementing Component for them with considerable code duplication.

what I have in mind is removing frame.inner.
this inverts ownership: instead of Frame<Something>, you would have Something { frame: Frame }.

you need impl Component for Something anyway, so this impl would also pass events to the Frame as appropriate.
but you don't need a SomeComplexScreen around the whole thing, because Something is already that

This commit enables registering function for updating footer and header
based on the content. This eliminates the need to create wrappers around
Frame to update them.

[no changelog]
@obrusvit obrusvit force-pushed the obrusvit/ui-mercury/footer-and-header-refactor branch from 8e161a0 to 4d2a4d3 Compare September 12, 2024 13:43
@obrusvit obrusvit added the translations Put this label on a PR to run tests in all languages label Sep 12, 2024
Copy link

legacy UI changes device test(screens) main(screens)

@obrusvit obrusvit merged commit 5980394 into main Sep 12, 2024
136 of 138 checks passed
@obrusvit obrusvit deleted the obrusvit/ui-mercury/footer-and-header-refactor branch September 12, 2024 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T3T1 Trezor Safe 5 translations Put this label on a PR to run tests in all languages
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants