Skip to content

Make debug windows right edge sticky - #455

Open
GrayHatter wants to merge 1 commit into
robinlinden:masterfrom
GrayHatter:sticky
Open

Make debug windows right edge sticky#455
GrayHatter wants to merge 1 commit into
robinlinden:masterfrom
GrayHatter:sticky

Conversation

@GrayHatter

Copy link
Copy Markdown
Contributor

Now when the right edge of any debug window in the list is at the edge
of the viewport width, it will stick there through resizes.

There's a bug where if the resize change is enough to "capture" a window
that wasn't intended to be sticky it will capture. Given this didn't
happen much during testing, I don't think the extra state + code
required is worth it for these windows.

@codecov

codecov Bot commented Jan 27, 2023

Copy link
Copy Markdown

Codecov Report

Base: 86.71% // Head: 86.71% // No change to project coverage 👍

Coverage data is based on head (2165393) compared to base (159409e).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #455   +/-   ##
=======================================
  Coverage   86.71%   86.71%           
=======================================
  Files          83       83           
  Lines        3981     3981           
=======================================
  Hits         3452     3452           
  Misses        529      529           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@GrayHatter
GrayHatter force-pushed the sticky branch 2 times, most recently from 650f702 to ed19a7f Compare January 29, 2023 19:47
Comment thread browser/gui/app.cpp Outdated
Comment thread browser/gui/app.cpp Outdated
Comment thread browser/gui/app.cpp Outdated
Comment thread browser/gui/app.cpp Outdated
NULL,
};

int delta = k_width - size.width;

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This is UB on the first call to this function since k_width isn't initialized.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Aren't integer members 0 if NOS in c++?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

No, unless set in e.g. a constructor they're uninitialized and reading from them is UB.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

good enough?

Comment thread browser/gui/app.cpp
break;
}
ImGuiWindow *win = ImGui::FindWindowByName(name);
if (!win) {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Isn't this a programming error if it ever happens? Until we have a saner way of keeping the names in sync, just assert() that we have the window we're looking for.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Isn't this a programming error if it ever happens?

No, because we might decide that windows are ephemeral. I can think of plenty of reasons we might want to destroy some # of them at runtime.

Assert will crash the app, window stickiness should be a "best effort" feature. No reason to make the app user hostile when it doesn't need to be.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Right now, all windows always exist. If that changes in the future it will be because a developer changes it and they probably need to have a look at this code and think about what they're doing.

Since you made me look at the imgui internals now, did you have a look at using GImGui.Windows for this so we don't have to hard-code a list of window names?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes/no?

Comment thread browser/gui/app.cpp Outdated
Comment thread browser/gui/app.h Outdated
Comment thread browser/gui/app.cpp Outdated
Now when the right edge of any debug window in the list is at the edge
of the viewport width, it will stick there through resizes.

There's a bug where if the resize change is enough to "capture" a window
that wasn't intended to be sticky it will capture. Given this didn't
happen much during testing, I don't think the extra state + code
required is worth it for these windows.
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