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

add functionality to add account buttons #147

Closed
wants to merge 4 commits into from

Conversation

kernelkind
Copy link
Contributor

This PR adds functionality to the "add account" button in the account switcher and the "add account" button in the account management view. While implementing this functionality, some problems with egui-nav were uncovered.

Navigating in the global window crashes:
image
after pressing the < Manage Account button:

thread 'main' panicked at /Users/redacted/.cargo/git/checkouts/egui-5e4507fa4153be06/fcb7764/crates/egui/src/widget_rect.rs:142:17:
Widget changed layer_id during the frame
stack backtrace:
   0: rust_begin_unwind
             at /rustc/6292b2af620dbd771ebb687c3a93c69ba8f97268/library/std/src/panicking.rs:661:5
   1: core::panicking::panic_fmt
             at /rustc/6292b2af620dbd771ebb687c3a93c69ba8f97268/library/core/src/panicking.rs:74:14
   2: egui::widget_rect::WidgetRects::insert
             at /Users/redacted/.cargo/git/checkouts/egui-5e4507fa4153be06/fcb7764/crates/egui/src/widget_rect.rs:142:17
   3: egui::context::Context::create_widget::{{closure}}
             at /Users/redacted/.cargo/git/checkouts/egui-5e4507fa4153be06/fcb7764/crates/egui/src/context.rs:1033:13
   4: egui::context::Context::write
             at /Users/redacted/.cargo/git/checkouts/egui-5e4507fa4153be06/fcb7764/crates/egui/src/context.rs:724:9
   5: egui::context::Context::create_widget
             at /Users/redacted/.cargo/git/checkouts/egui-5e4507fa4153be06/fcb7764/crates/egui/src/context.rs:1027:9
   6: egui::ui::Ui::new
             at /Users/redacted/.cargo/git/checkouts/egui-5e4507fa4153be06/fcb7764/crates/egui/src/ui.rs:116:9
   7: egui_nav::Nav<T>::show
             at /Users/redacted/.cargo/git/checkouts/egui-nav-f3bee59503678f4e/a8dd95d/egui-nav/src/lib.rs:350:26
   8: notedeck::ui::global_popup::DesktopGlobalPopup::show::{{closure}}
             at ./src/ui/global_popup.rs:25:17
   9: notedeck::ui::fixed_window::FixedWindow::show::{{closure}}
             at ./src/ui/fixed_window.rs:53:28
             
...

I think this is happening because the underlying window in FixedWindow has LayerId Order::Middle, but in egui-nav/src/lib.rs:353 we're setting the LayerId to Order::Background in the same frame

@jb55 I'm not very familiar with how egui-nav works, what are your thoughts on how to fix this?

@jb55
Copy link
Contributor

jb55 commented Jul 4, 2024

hmm ok this seems like a simple fix, I'm not 100% how the layers work but we can just ensure they are the same.

@jb55
Copy link
Contributor

jb55 commented Jul 4, 2024

try fast-forwarding your branch to:

egui-nav-fix

there seems to be a clipping issue but it doesn't crash at least

@kernelkind
Copy link
Contributor Author

it's very strange because I can confirm that egui-nav-fix doesn't crash like it did previously when building for release, but when building for debug it still crashes...

@kernelkind
Copy link
Contributor Author

is the 'clipping' issue you're referring to the way the window resizes in 1 frame instead of animating the resize? @jb55 What currently happens is egui-nav animates, and when it's finished the window resizes in 1 frame.

Ideally I think the back animation and window resize would happen in one animation. But that might entail moving the window to egui-nav...

Would doing the back animation and then animating the window resize (two separate steps instead of one) be sufficient?

@jb55
Copy link
Contributor

jb55 commented Jul 5, 2024 via email

@kernelkind
Copy link
Contributor Author

kernelkind commented Jul 5, 2024

There are lots of issues with the current window, like different sizes
between the manager and the account creation. Ideally each control
should just fill the container it is given, I think the account
creation screen has hardcoded-nonresponsive sizes?

The variable sizing of the global window is by design. I chose to implement it that way because the login panel looks best in the dimensions laid out in the figma design (in my opinion) and other views might not (like account management).

These are the things I'm imagining we're using the global window for:

  • account login
  • settings
  • other user profile
  • your profile
  • posting
  • add column
  • account management

some things will require more area than others, so the global window should adapt to present the view best in my opinion.

what do you think? @jb55

@jb55
Copy link
Contributor

jb55 commented Jul 5, 2024

The variable sizing of the global window is by design. I chose to implement it that way because the login panel looks best in the dimensions laid out in the figma design (in my opinion) and other views might not (like account management).

I think that makes sense logically but when you are navigating between windows with different sizes it feels jarring and is terrible design wise.

@kernelkind
Copy link
Contributor Author

kernelkind commented Jul 5, 2024

I think that makes sense logically but when you are navigating between windows with different sizes it feels jarring and is terrible design wise.

I agree right now it is quite jarring. I think a clever animation could help.

@kernelkind kernelkind closed this Sep 19, 2024
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