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

BackgroundPainter called in unexpected order #79

Open
remikey opened this issue Oct 15, 2019 · 2 comments
Open

BackgroundPainter called in unexpected order #79

remikey opened this issue Oct 15, 2019 · 2 comments
Labels
Milestone

Comments

@remikey
Copy link
Member

remikey commented Oct 15, 2019

When using several BackgroundPainters (several call to Topology.addBackgroundPainter()), we would expect that the last added be called last (at it is the case when handling LinkPainter and NodePainter). It is not the case.

Please correct this.

@remikey remikey added the bug label Oct 15, 2019
@remikey remikey added this to the next milestone Oct 15, 2019
@acasteigts
Copy link
Member

As we discussed this morning, for now, one (easy) option is to insert background painters in penultimate position when Topology.addBackgroundPainter()) is called (the last position being used by an inner painter that draws the sensing ranges). If you feel comfortable with this, then it is fine with me. Otherwise, we can discuss further.

@remikey
Copy link
Member Author

remikey commented Oct 16, 2019

Although I first liked the idea of this elegant quick fix, after thinking about it for some time yesterday, I can see painful drawbacks to this approach:

  • Since the user is allowed to remove/replace the "inner painter" (using Topology.setDefaultBackgroundPainter()), it would require that we make sure that the last painter is our painter.
    Both of the following cases are relevant and would complicate understanding of the painters mechanism:

    • Using JBackgroundPainter.class:
      Any implementation trying to enhance the JBackgroundPainter capabilities by inheriting from it (as it is done with the JBackgroundPainterHD by adding anti-aliasing) would not be considered as the "inner painter" and thus, not displayed on top.
    • Using instanceof JBackgroundPainter:
      Any lazy implementation done by inheriting from JBackgroundPainter, instead of implementing BackgroundPainter, would be considered as the original inner painter and thus displayed on top, although it is expected to be last.
  • It would make the code harder to read (and harder the maintenance).

Discussion

I think that the issue is that we use a BackgroundPainter to draw something that we actually want on top of anything on the background (the LinkPainter layer has naturally been placed in the right position). The current content of our JBackgroundPainter makes it more a JSensingRangePainter.

This could be solved by adding a layer drawn after the BackgroundPainter layer (and before or after the LinkPainter layer).
Possible alternatives (any of which would make things clearer):

  1. I am tempted to add something like an IntermediatePainter that would fix this problem and future ones, but I suppose that we might end up with the same kind of problem with the default sensing range being drawn after/before something else (?).

  2. A dedicated layer for a SensingPainter interface can also be created. This would fix this specific problem.

  3. The combination of both would make sure this is fixed and might prevent future issues.

@pictavien pictavien modified the milestones: v1.2.0, unplanned Feb 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants