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

Shell: windows implementation from content_insets #1592

Merged
merged 1 commit into from
Feb 17, 2021

Conversation

HoNile
Copy link
Contributor

@HoNile HoNile commented Feb 16, 2021

With this dropdown should be better on windows.

There is still an issue with HiDPI screen, it doesn't look like using the scale factor in this function helps.

@Zenthae
Copy link

Zenthae commented Feb 16, 2021

longing for it ^^, #1589

@cmyr
Copy link
Member

cmyr commented Feb 17, 2021

I won't be able to test this until tomorrow afternoon at the earliest. If anyone else on windows is able to do a review I'm happy to delegate approval to them :)

@Zenthae
Copy link

Zenthae commented Feb 17, 2021

i don't know if i should post it here but, WindowSizePolicy::Content behave strangely when no size is set, it start the window with a 0, 0 size and you need to resize it manually to make it take the policy into account, and well, it wouldn't work at all if resize is set to false

@cmyr
Copy link
Member

cmyr commented Feb 17, 2021

@rjwittams any thoughts on this size business?

@rjwittams
Copy link
Collaborator

I don't know off the top of my head. I do think that without the content offsets we could end up iteratively shrinking the window. I don't have windows currently to test with.

@HoNile
Copy link
Contributor Author

HoNile commented Feb 17, 2021

@Zenthae did you have an example to reproduce this ? Myself I test using WindowSizePolicy::Content, with no size set on the tree example from nursery and it work fine.

@cmyr
Copy link
Member

cmyr commented Feb 17, 2021

Okay I'm in front of a computer where I could test this, if anyone has any suggestions for what I should be testing?

@HoNile
Copy link
Contributor Author

HoNile commented Feb 17, 2021

During my tests I checked the dropdown example from nursery, previously the sub_windows didn't pop where there should (and it's still the case if you use HiDPI monitor). And I tested WindowSizePolicy::Content on tree as explain, I don't know if there is better tests to do.

Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

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

This looks correct to me; content_insets is supposed to return a pixel value, so it's possible that elsewhere there is something that is working in logical units and needs to be converted to pixels?

This is actually a bit iffy, though, I would expect window sizes to be in logical units, and not be display dependant, but this does at least do what the docs say it does.

Thanks!

druid-shell/src/platform/windows/window.rs Outdated Show resolved Hide resolved
@cmyr
Copy link
Member

cmyr commented Feb 17, 2021

Thanks!

@cmyr cmyr merged commit f3b534b into linebender:master Feb 17, 2021
@Zenthae
Copy link

Zenthae commented Feb 17, 2021

@Zenthae did you have an example to reproduce this ? Myself I test using WindowSizePolicy::Content, with no size set on the tree example from nursery and it work fine.

it's the beginning of a new app i'm doing. i used this as a test

use druid::{
    widget::{Label, Padding},
    AppLauncher, Data, Lens, Widget, WindowDesc, WindowSizePolicy,
};

#[derive(Debug, Default, Data, Clone, Lens)]
struct AppState {}

fn ui_builder() -> impl Widget<AppState> {
    Padding::new(5., Label::new("Hello Bluetooth monitor"))
}

fn main() -> windows::Result<()> {
    let main_window = WindowDesc::new(ui_builder())
        .title("Bluetooth Monitor")
        // .window_size((100.0, 40.0))
        .window_size_policy(WindowSizePolicy::Content);

    AppLauncher::with_window(main_window)
        .use_simple_logger()
        .launch(AppState::default())
        .expect("Failed to start the app");

    Ok(())
}

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.

4 participants