Bugfix/469 overlay stack interface

[toaster]

Description:

In preparation for a fix of #469 this PR introduces a new overlay interface to fyne.Canvas. The old methods (Overlay, SetOverlay) are deprecated.

Checklist:

• [ ] Tests included.
• Lint and formatter run with no errors.
• Tests all pass.

Where applicable:

• Public APIs match existing style.
• Any breaking changes have a deprecation path or have been discussed.

[andydotxyz]

Is there a way to do this without adding the long list of methods to the Canvas interface?

One more specific concern is that PopOverlay looks dangerous - for example something that pushes and later pops may be popping the wrong item. I like that RemoveOverlay removes all the child ones too, which makes me think that perhaps AddOverlay and RemoveOverlay is sufficient.

Back to the point above maybe Overlays() returns an OverlayStack or something? That is basically how it's implemented under the hood and would mean a single new method added to the interface. Just a thought.

Or even further outside of the box... Is it a ContentStack? The lowest level is the real content and the rest are overlays... but I just wondered are they not really the same thing?

[toaster]

I'm not sure about the ContentStack what would be the benefits? How will the API look like? Would window.SetContent() still be in place?
Honestly, I think it would be better to not mix content and overlays for now.

Regarding func Overlays() *OverlayStack:
Do you mean that the canvas will not have methods for directly adding/removing the overlays?
So canvas.PushOverlay(x) will become canvas.Overlays().Push(x)?

Regarding PopOverlay(): I thought about this too. I just kept it because I didn't wanted to change NewPopUpMenuAtPosition :). So for me the three methods AddOverlay(x), RemoveOverlay(x) and TopOverlay() would be sufficient.
Accessing these via canvas.Overlays() is acceptable for me, too.

[andydotxyz]

Yeah ContentStack was a bit of a curve ball - SetContent() would basically be like ContentStack().Remove(oldContent); ContentStack().Add(newContent) - causing all overlays open to be dismissed like your current RemoveOverlay(). For some reason I imagined it could make some driver code simpler as it would remove the divide between content and overlay content. Maybe I was just getting carried away here :).

Yes, the Overlays() *OverlayStack would indeed then have the Push/Remove etc methods on it. I don't know why I thought of it, probably just to reduce the number of new calls on Canvas() when they would probably all be implemented by the same stack code anyway...

Your thoughts on PopOverlay make sense, but I think in this instance bigger changes to the existing widgets allow for a cleaner and less ambiguous API, so let's do that :)

[andydotxyz]

Is it intended that this version of the stack only has 1 element? (or maybe I am reading it wrong?)
For the location of the stack implementation I thought we were avoiding new files directly in internal/?

Also, and I'm not sure about this: Given the methods on StackOverlay refers to overlays should they be just Add(), Remove() and Top()? The code using them can seem quite repetitive otherwise? (a bit more of an open question here).

[toaster]

Yes, the real stack implementation will follow in a separate PR.
This PR should only introduce the necessary interfaces without changing anything on the functionality. It is meant as a “refactoring”.

I can put the the stack implementation deeper but not into driver directly because the test package depends on it and the driver package still depends on widget (because of #707) and widget tests are within the widget package which depends on driver.
I didn't do so though because I didn't wanted to introduce a utility package. WDYT?

I'll do the method name length reduction.

[andydotxyz]

Thanks. I guess the file can stay where it is until we get a solution to the circular dep, or find a nice sub-package for such things.

I had not thought about "All()" instead of "Overlays()" - I wonder if that implies an unsorted collection though, would "List()" be more explicit? (Maybe this is not an issue with Go where there is no "set" primitive?)