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

dyn_stack problem with Option<MenuDisplay> #354

Closed
aalhitennf opened this issue Mar 7, 2024 · 5 comments
Closed

dyn_stack problem with Option<MenuDisplay> #354

aalhitennf opened this issue Mar 7, 2024 · 5 comments

Comments

@aalhitennf
Copy link
Contributor

aalhitennf commented Mar 7, 2024

Screenshots that explains the problem: https://imgur.com/a/C1ibQaH

I tracked down the issue to dyn_stack at src/window_handle.rs line 1471. dyn_stack takes in proper amount of menu items from context_menu_items, but iterates less in view_fn.

In src/views/dyn_stack.rs when opening the context menu

let items = items.into_iter().collect::<SmallVec<[_; 128]>>();
let hashed_items = items.iter().map(&key_fn).collect::<FxIndexSet<_>>();
// 16 entries and 3 separators ->  items.len() = 19,  hashed_items.len()  = 17
// 16 entries and 4 separators -> items.len() = 20,  hashed_items.len()  = 17
// 16 entries and 5 separators -> items.len() = 21,  hashed_items.len()  = 17 

So collecting to FxIndexSet seems to remove (amount of separators - 1) from the end of total menu items.

I tried using default hasher at src/views/dyn_stack.rs line 16:

pub(crate) type FxIndexSet<T> = indexmap::IndexSet<T, BuildHasherDefault<std::hash::DefaultHasher>>;

but that didn't help.

@jrmoulton
Copy link
Contributor

jrmoulton commented Mar 7, 2024

https://github.com/lapce/floem/blob/936e5ce26efaef9a9ab2c8f6fce3bfe09f9a2c21/src/window_handle.rs#L1392C20-L1398C22

Looks like this part of the code should use an atomicu32 for key function instead of doing s.clone().

The clone is problematic because the separator items will hash the same so as soon as there is more than one the dyn stack will have the count wrong. Unique values are needed

@aalhitennf
Copy link
Contributor Author

aalhitennf commented Mar 7, 2024

f for enter misclick 😮‍💨

I fixed it by changing menu display to enum

20240307_21h01m26s_grim

    #[derive(Clone, PartialEq, Eq, Hash)]
    pub enum MenuDisplay {
        Separator(usize),
        Item {
            id: Option<u64>,
            enabled: bool,
            title: String,
            children: Option<Vec<MenuDisplay>>,
        },
    }

separator is is given at format_menu

    fn format_menu(menu: &Menu) -> Vec<MenuDisplay> {
        menu.children
            .iter()
            .enumerate()
            .map(|(sep, e)| match e {
                crate::menu::MenuEntry::Separator => MenuDisplay::Separator(sep),

@aalhitennf aalhitennf reopened this Mar 7, 2024
@aalhitennf
Copy link
Contributor Author

Could also make MenuEntry::Separator to have id from static AtomicU64 when its added to Menu but dunno if that's necessary, index from iterator should be unique enough?

I can make PR, whatever you think is better?

@jrmoulton
Copy link
Contributor

The enum looks like a good solution to me. @dzhou121 ?

aalhitennf added a commit to aalhitennf/floem that referenced this issue Mar 7, 2024
@aalhitennf
Copy link
Contributor Author

Made a pr

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

No branches or pull requests

2 participants