Implement Eq and Hash for PopupSurface

[Antikyth]

Does what it says on the tin. This allows PopupSurface to be used in HashMaps, should compositors wish to associate custom state with a popup.

Closes #1199.

[codecov-commenter]

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (2dedd2e) 21.98% compared to head (8d867dd) 21.99%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1200   +/-   ##
=======================================
  Coverage   21.98%   21.99%           
=======================================
  Files         154      154           
  Lines       24093    24097    +4     
=======================================
+ Hits         5298     5301    +3     
- Misses      18795    18796    +1     

Flag	Coverage Δ
wlcs-buffer	18.98% <0.00%> (-0.01%)	⬇️
wlcs-core	18.66% <0.00%> (-0.01%)	⬇️
wlcs-output	7.74% <0.00%> (-0.01%)	⬇️
wlcs-pointer-input	20.78% <0.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
src/wayland/shell/xdg/mod.rs	31.36% <0.00%> (-0.15%)	⬇️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[kchibisov]

Suggested change

// self.alive().hash(state);

[Antikyth]

This matches above where it is commented out in the PartialEq implementation. Since Hash has to be kept in sync with the PartialEq implementation, either the commented out part should be entirely removed, or this should remain in case it is un-commented-out.

[kchibisov]

having dead commented out code looks like an artifact, so it must be removed.

[Drakulix]

So the commented out alive check is actually the smoking gun, telling us, why this implementation is not possible.
It used to be there, because we didn't want any comparison with dead popups to succeed and it's commented out, because it is actually not needed.

Wayland objects (which includes the WlSurface and XdgPopup types the PopupSurface is composed of) don't implement Eq, because wayland-rs doesn't guarantee that dead objects compare as equal, even if they refer to the same dead object.

Which violates the invariant of Eq, which disallows Hash. So these implementations are unsafe and you simply can't construct a HashMap using this type as a key, sorry.

[kchibisov]

So the commented out alive check is actually the smoking gun, telling us, why this implementation is not possible.
It used to be there, because we didn't want any comparison with dead popups to succeed and it's commented out, because it is actually not needed.

Commenting out code is not a good way to document anything. You can always write something and explain why something doesn't work. You can also add doc comments to trait impl methods explaining that.

Like commented out code should never be the case in nearly any language, it's like the worst.

[Drakulix]

Commenting out code is not a good way to document anything. You can always write something and explain why something doesn't work. You can also add doc comments to trait impl methods explaining that.

I don't disagree, but I also don't remember how the code ended up this way. It certainly wasn't meant to be documentation, I guess this happened during the large wayland 0.30 PRs and this was simply overlooked.

I just mentioned it, because it prompted me to lookup the wayland-client PartialEq implementation.

[Drakulix]

@Antikyth first of all thank you for the contribution. Can you maybe explain your use-case for this a bit more? I am trying to think of a good alternative to suggest, given that this code won't work as is, and I think would help knowing what kind of data you are storing.