style: promote idiomatic functional Rust

[michalfita]

I know this is unsolicited request, but as you're proud in writing Cosmic Desktop in Rust would be nice to promote more idiomatic functional style with less mutability. I don't mind if you throw this away as unsuitable for your needs.

Additionally I expanded the menu row to increase space between menu item name and the shortcut as translated items were too tight visually.

Working on this, however, led me to form following questions:

• This code demonstrates a lot of tedious chiseling to built context menu, but doesn't see to use iced_aw. Is there reason to build UI this way? Is there a roadmap what libcosmic would include in terms of standard desktop widgets?
• I found it surprising to keep mapping between key binding and action kept in HashMap<> to be iterated over to find key binding for given action. Would it make sense to crate a type like UIActionMap that would hold some faster hash maps for both directions? For several key bindings in the application actually a vector with iterative search is faster than HashMap as actual hashing and bucketing takes plenty of cycles.
• Is the font final choice?

[leb-kuchen]

Congrats, you made the code less readable and extensible.

[michalfita]

@leb-kuchen I'd be glad if you could elaborate on that.

[Kartonrealista]

This replaces one for loop with two

[michalfita]

It'd been proven many times Rust's functional style gives compiler much wider field for optimisations than traditional imperative loops, especially if mutability is removed.

I can't run this through godbolt as they don't have criterion available, but my benchmark for 1000000 items, shows 2.7281 ms for for loop and 471.58 µs for filter()/count() iterators on my notebook PC. For 10000 difference is smaller, but still filter()/count() wins: 6.3825 µs / 4.7141 µs.

Style wise, I'd personally replace tuple with own type supporting addition to right fields and logic queries, etc.

[leb-kuchen]

This now O(n^2) instead of O(n).

[leb-kuchen]

Wait, are you folding over an option

[leb-kuchen]

Go write Haskell

[joshuamegnauth54]

I've had similar questions when I first started contributing to COSMIC. Don't take these answers as official. They're simply how I understand the situation.

This code demonstrates a lot of tedious chiseling to built context menu, but doesn't see to use iced_aw.

iced_aw and libcosmic depend on different versions of iced so (I think) it'd be difficult to use for COSMIC until the two branches of iced converge.

Is there reason to build UI this way? Is there a roadmap what libcosmic would include in terms of standard desktop widgets?

libcosmic implements its own set of widgets which are preferred because the team has full control over them. iced is a toolkit to build toolkits, so it is comparatively Spartan in relation to GUI libraries like GTK or Qt. I'm not sure if there's a roadmap for widgets that the library will provide, but there are open issues for widget improvements or new widgets in the libcosmic repo itself.

but as you're proud in writing Cosmic Desktop in Rust would be nice to promote more idiomatic functional style with less mutability
[...]
I found it surprising to keep mapping between key binding and action kept in HashMap<> to be iterated over to find key binding for given action. Would it make sense to crate a type like UIActionMap that would hold some faster hash maps for both directions? For several key bindings in the application actually a vector with iterative search is faster than HashMap as actual hashing and bucketing takes plenty of cycles.

COSMIC's development has been extremely fast. Some of the code quality issues you've mentioned is due to that alacritous dev time where finished features or bug fixes took precedence over tightly bound code. You'll notice throughout most of the codebases that there are TODOs to move certain implementations over to libcosmic or to update the code to use new libcosmic features. There are instances of duplicated features (e.g. localization or the key bindings) across the codebases that will likely be fixed later as the team figures out permanent solutions. I'm a noob so I tend to leave questions like that to the main developers. 😂

[michalfita]

Method	Avg t @ 10000 elements
original for	6.4166 µs
filter()/count() (supposed O(n²)	4.5204 µs
filter()/fold() (last one)	3.6868 µs

[dengelt]

The vec! macro is not needed here, plain arrays implement IntoIterator as well.

Also, wouldn't the use of extend versus a series of pushes make it less convenient to conditionally add menu items, like (currently pending) PRs like #359 and #370 do?

I guess I don't see the benefit of using extend. If it is to reduce the number of re-allocations as the children vec grows, this should already be solved by initializing with with_capacity.

[michalfita]

Point taken. This to me screams for kind of builder pattern, with something like .add_item() and .add_item_if() flat extensible list. Example:

let menu = MenuBuilder::new()
    .add_item(fl!("open"), Action::Open)
    .add_item_if(fl!("open-with"), Action::OpenWith, selected.exactly_one())
    .build()

Don't hesitate to tell me if I'm hallucinating 😉

[dengelt]

Personally, for what the code does now this would be too much abstraction. After all, right now all the builder would do is add elements to a vector. I don't know what the "minimum number of builder methods to warrant a builder pattern" would be, but I think it's more than 2.

Also if I think about other places where builders are used, it's usually places where the creation is a complex process that needs to preserve certain invariants for every possible construction, and where different users create differently built objects. But here, there would only be one call to build(), because this is a very internal use, compared to builders that are part of the public API of HTTP libraries, database query builders, command line parsers, etc...

If this was in libcosmic, this should definitely be a builder pattern.

But ultimately, this is a decision someone from the pop-os team should make.

[michalfita]

I got the feedback for my proposal that I considered and I have some ideas how to tackle certain things in this code to make it more long term maintainable, but I have to start from scratch.