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

fix: maximizing window on linux, closes #442 #456

Merged
merged 3 commits into from
Jul 11, 2022

Conversation

keiya01
Copy link
Member

@keiya01 keiya01 commented Jun 29, 2022

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Docs
  • New Binding issue #___
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change?

  • Yes, and the changes were approved in issue #___
  • No

Checklist

  • When resolving issues, they are referenced in the PR's title (e.g fix: remove a typo, closes #___, #___)
  • A change file is added if any packages will require a version bump due to this PR per the instructions in the readme.
  • I have added a convincing reason for adding this feature, if necessary

Other information

@keiya01 keiya01 requested a review from a team as a code owner June 29, 2022 00:11
@keiya01 keiya01 assigned keiya01 and unassigned keiya01 Jun 29, 2022
@amrbashir
Copy link
Member

Just tried this PR and the bug still exists

Here is my repro:

// Copyright 2019-2021 Tauri Programme within The Commons Conservancy
// SPDX-License-Identifier: Apache-2.0

use tao::{
  event::{Event, WindowEvent},
  event_loop::{ControlFlow, EventLoop},
  window::WindowBuilder,
};

#[allow(clippy::single_match)]
fn main() {
  let event_loop = EventLoop::new();

  let mut window = Some(
    WindowBuilder::new()
      .with_title("A fantastic window!")
      .with_inner_size(tao::dpi::LogicalSize::new(1080, 1980))
      .with_maximized(true)
      .with_resizable(false)
      .build(&event_loop)
      .unwrap(),
  );

  event_loop.run(move |event, _, control_flow| {
    *control_flow = ControlFlow::Wait;
    println!("{:?}", event);

    match event {
      Event::WindowEvent {
        event: WindowEvent::CloseRequested,
        window_id: _,
        ..
      } => {
        // drop the window to fire the `Destroyed` event
        window = None;
      }
      Event::WindowEvent {
        event: WindowEvent::Destroyed,
        window_id: _,
        ..
      } => {
        *control_flow = ControlFlow::Exit;
      }
      Event::MainEventsCleared => {
        if let Some(w) = &window {
          w.request_redraw();
        }
      }
      _ => (),
    }
  });
}
```

@keiya01 keiya01 force-pushed the fix/maximize-window-linux branch from f4a4b7d to c326771 Compare July 4, 2022 04:21
@keiya01
Copy link
Member Author

keiya01 commented Jul 4, 2022

@amrbashir
I fixed the above error. Please check the following commit 🙇
c326771

@keiya01 keiya01 changed the title fix: miximizing window on linux, closes #442 fix: maximizing window on linux, closes #442 Jul 5, 2022
@amrbashir
Copy link
Member

the last commit brought more issues than it solved. If you you set the size to 100,200 and set resizable to false and maximized to true, it will fill the whole screen which it shouldn't.

Here is some scenarios with the expected outcome to help you debug this further:

  •   {
      	"width": 100,
      	"height": 200,
      	"resizable": true,
      	"maximized": true,
      }

    the window should open maximized but when you unmaximize it, it should restore its size to 100, 200 and the window can be resized using the mouse

  •   {
      	"width": 100,
      	"height": 200,
      	"resizable": false,
      	"maximized": true,
      }

    the window should open maximized but when you unmaximize it, it should restore its size to 100, 200 and the window can NOT be resized using the mouse

  •   {
      	"width": 100,
      	"height": 200,
      	"resizable": true,
      	"maximized": false,
      }

    the window should NOT open maximized and its size should be 100, 200 and can be resized using the mouse

  •   {
      	"width": 100,
      	"height": 200,
      	"resizable": false,
      	"maximized": false,
      }

    the window should NOT open maximized and its size should be 100, 200 and can NOT be resized using the mouse

@keiya01
Copy link
Member Author

keiya01 commented Jul 6, 2022

@amrbashir
Thank you for your help 🙏

If you you set the size to 100,200 and set resizable to false and maximized to true, it will fill the whole screen which it shouldn't.

Please tell me about the above sentence.
I think my latest commit is filling window with resizable: false and maximized: true, but shouldn't we fill window in this senario?

@keiya01
Copy link
Member Author

keiya01 commented Jul 6, 2022

And I forgot to fix maximization in runtime, so I will fix it...

@amrbashir
Copy link
Member

I think my latest commit is filling window with resizable: false and maximized: true, but shouldn't we fill window in this senario?

My bad I mixed them up. While I was testing, I had resizable true and maximized false and it maximized the window which it shouldn't.

just try to match the 4 scenarios I posted above with the expected outcome while creating the window and in runtime.

@keiya01
Copy link
Member Author

keiya01 commented Jul 6, 2022

My bad I mixed them up. While I was testing, I had resizable true and maximized false and it maximized the window which it shouldn't.

In my environment, when I had resizable true and maximized false, the window is not maximized.
Is this case occurred in runtime?

@amrbashir
Copy link
Member

amrbashir commented Jul 6, 2022

No, it was when creating the window. I'll do more testing tomorrow.

@amrbashir
Copy link
Member

I did some more testing and noted down the issues I found below but I am not sure if these are bugs or the desired behavior.

maximized resizable
true true works perfectly
false true works perfectly
true false can't be UNmaximized at runtime
false false can't be maximized at runtime

@keiya01
Copy link
Member Author

keiya01 commented Jul 6, 2022

Thank you for your investigation. Yeah I'm fixing runtime behavior.
And I think maximized true and resizable false senario should work but I will check if this senario works on other platform.

@keiya01
Copy link
Member Author

keiya01 commented Jul 6, 2022

I tested these cases on macOS and windows.
On macOS these are working great but on windows these are not working...
But I think maximized: true, resizable: false should work because this is easy to understand intuitively.

Copy link
Member

@amrbashir amrbashir left a comment

Choose a reason for hiding this comment

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

I think this PR is good enough but will let @wusyong decide whether he want to merge or not

@keiya01
Copy link
Member Author

keiya01 commented Jul 6, 2022

Do we need to support maximizing window on windows at initialization?
If you agree, I will fix it in another PR.

@amrbashir amrbashir requested a review from wusyong July 6, 2022 12:13
@amrbashir amrbashir merged commit 01fb1d6 into dev Jul 11, 2022
@amrbashir amrbashir deleted the fix/maximize-window-linux branch July 11, 2022 23:42
@github-actions github-actions bot mentioned this pull request Jul 11, 2022
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.

[bug] Positioning window with config does not work under certain conditions
2 participants