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

Add debug option to display widget ids #876

Merged
merged 3 commits into from
May 12, 2020
Merged

Add debug option to display widget ids #876

merged 3 commits into from
May 12, 2020

Conversation

cmyr
Copy link
Member

@cmyr cmyr commented Apr 24, 2020

This will paint the id of each widget of some subtree in the bottom
right of that widget's bounds; useful for debugging various bits
of event flow and widget behaviour.

Screen Shot 2020-04-24 at 1 32 24 PM

@luleyleo
Copy link
Collaborator

Good idea 👍

But this can cause overlapping ids:
2020-04-24_20-24

Not sure how to deal with this but I would not like it to stay like this.
I almost did not notice that little red between the numbers and thought it would be 115.

To reproduce:

  1. enable this for the view_switcher example
  2. choose View 1 "Simple Button"
  3. choose View 3
  4. repeat from 2. until the ids are >100
  5. choose View 1 or View 4 to get the overlapping ids

@cmyr
Copy link
Member Author

cmyr commented Apr 24, 2020

yea, I was just working on that. It's kind of tricky; the easiest thing I can do is use z-order painting, so at least we prioritize the topmost widget. I'm also experimenting with only painting if we're currently hot. Not sure what combination of these things makes the most sense...

@cmyr
Copy link
Member Author

cmyr commented Apr 24, 2020

2020-04-24 15 58 48

Okay, I've changed the behaviour to paint using z-order, so children overwrite parents; this should also fix the issue of smaller labels being written over larger labels, because children should always have larger ids than parents. (const ids might violate this, but I don't want to worry about that yet).

In addition, we now only paint the label if the widget is hot.

@luleyleo
Copy link
Collaborator

The new approach feels better 👍
It does seem like it draws a bit outside the widget tho, which causes visual artifacts as only the widget gets invalidated.
2020-04-24_22-33

@luleyleo
Copy link
Collaborator

The invalidation (I guess) also causes problems with the Current view: x label in the view_switcher example. It sometimes does not get a label when hovering or the label is not removed when leaving the label.

@cmyr
Copy link
Member Author

cmyr commented Apr 25, 2020

right, we're stroking the boundary so it may spill. can adjust that.

about the view_switcher example.... that's weird. My spidey sense says this is an existing bug that we're just seeing for the first time, but I'll dig in a bit more tomorrow.

@luleyleo luleyleo added the S-waiting-on-author waits for changes from the submitter label Apr 28, 2020
@cmyr
Copy link
Member Author

cmyr commented May 8, 2020

Okay fixed this, was an issue with how I was handling hot changes.

@cmyr cmyr added S-needs-review waits for review and removed S-waiting-on-author waits for changes from the submitter labels May 8, 2020
Comment on lines 366 to 367
if debug_layout {
self.debug_paint_layout_bounds(&mut inner_ctx, env);
}
if debug_ids {
self.debug_paint_widget_ids(&mut inner_ctx, env);
}
Copy link
Member

Choose a reason for hiding this comment

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

The debug_paint_widget_ids method might call debug_paint_layout_bounds, so I think we can optimize it into something like this:

// Check the bool first, because most widgets aren't hot and we won't need to do the map lookup
let debug_ids = inner_ctx.is_hot() && env.get(Env::DEBUG_WIDGET_ID);

let debug_layout = if debug_ids {
    // Change debug_paint_widget_ids to return true if it called debug_paint_layout_bounds.
    // Then we won't need to call it again or even do the env map lookup.
    !self.debug_paint_widget_ids(&mut inner_ctx, env) && env.get(Env::DEBUG_PAINT)
} else {
    env.get(Env::DEBUG_PAINT)
};
if debug_layout {
    self.debug_paint_layout_bounds(&mut inner_ctx, env);
}

Copy link
Member Author

Choose a reason for hiding this comment

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

okay, I think we can assume that debug_paint_widget_ids always paints the layout so I can simplify this a bit further, overall looks reasonable though.

Copy link
Member

@xStrom xStrom left a comment

Choose a reason for hiding this comment

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

Looks good and useful.

@xStrom xStrom removed the S-needs-review waits for review label May 11, 2020
Copy link
Collaborator

@luleyleo luleyleo left a comment

Choose a reason for hiding this comment

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

This breaks GTK invalidation. Only some parts get invalidated, buttons for example do not indicate hover, or do so only in one corner. Text field selection does not show up, but while typing text it does work.

This only happens when debug_widget_id is active.

It happens even without debug_widget_id.

After merging master (which gave me a conflict, wtf?), I tried X11 and it works fine. So this really seems to be GTK specific.

@luleyleo
Copy link
Collaborator

So, after that merge conflict irritated me, I forcefully reset my local git repo and now the rendering bug is gone 🙈 . But it still does not seem to work entirely, some widgets do not show their id, or have it offset (which could cause some ids to be offscreen)

@luleyleo
Copy link
Collaborator

luleyleo commented May 11, 2020

After comparing the behavior with X11 (where it works correctly) in the calc example, it seems like ids are drawn to far below and right of the widget they belong to. Also they sometimes do not show up at all, probably because the place where they draw is not being invalidated.

@xStrom
Copy link
Member

xStrom commented May 11, 2020

I tested this on my Ubuntu 20.04 just now and both X11 and GTK work just fine. There are no rendering bugs and the ids are drawn in the correct location.

Maybe there's something wrong with your setup @Finnerale? Give cargo clean a try.

@luleyleo
Copy link
Collaborator

Neither cargo clean nor a complete re-clone of the repo changed it.

@luleyleo
Copy link
Collaborator

I can even reproduce this on my Ubuntu 20.04 laptop.

@cmyr
Copy link
Member Author

cmyr commented May 11, 2020

hm, so something like this was happening, but I think I've addressed it in an update? is it possible you're on a commit other than 3396138?

@luleyleo
Copy link
Collaborator

Nope, it is 3396138 . Like I said, I've deleted my local copy of druid and cloned it again, compiled it fresh and the issue still is there, on my Arch desktop and on my Ubuntu 20.04 laptop, both with up to date system and Rust.

@cmyr
Copy link
Member Author

cmyr commented May 11, 2020

that's really strange, because I had an issue similar to what you'd described, but it's now fixed. Can you share a screencap?

cmyr added 3 commits May 11, 2020 17:26
This will paint the id of each widget of some subtree in the bottom
right of that widget's bounds; useful for debugging various bits
of event flow and widget behaviour.
This ensures that the ids of children will not be obscured by
the ids of their parents.

This also adds caching of the textlayout object.
@luleyleo
Copy link
Collaborator

So, I've found the reason why only I got it: It happens on Wayland, not on X11.

I've added two videos, the first is on Wayland, the second (same computer) on X11.
wayland.webm
x11.webm

@xStrom
Copy link
Member

xStrom commented May 12, 2020

I managed to reproduce the issue with Wayland. This problem has nothing to do with this PR though, the same problem already exists with the anim example. It seems to be related to paint_with_z_index and can be addressed separately from this PR here. That function internally calls self.render_ctx.current_transform() which returns a different value on Wayland.

Copy link
Collaborator

@luleyleo luleyleo left a comment

Choose a reason for hiding this comment

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

Interesting, I thought it would have been introduced in this PR because when I tested this PR the last time, it did work. In that case I'm happy to get this merged.

@cmyr
Copy link
Member Author

cmyr commented May 12, 2020

ah that makes sense, I had a similar issue on the coregraphics background where I couldn't trust the current transform returned by the platform, and have to track it separately myself.

@cmyr cmyr merged commit 0d56631 into master May 12, 2020
@cmyr cmyr deleted the debug-widget-id branch May 12, 2020 14:52
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.

3 participants