1220873 - Convert Layer::mVisibleRegion to a LayerIntRegion

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Botond Ballo [:botond]]

Bug 1043013 introduced strongly-typed region classes, but there's a lot of code that still uses the old, untyped classes, that I'd like to switch over to the strongly-typed classes.

Switching Layer::mVisibleRegion is a good start. Its type after the switch would be LayerIntRegion. LayerComposite::mShadowVisibleRegion should probably be switched at the same time (also to LayerIntRegion).

This would make a good mentored bug.

Comment 1

[Sunny Sidhu]

Hi Botond,

Could you provide me some pointers or how to start please?

Comment 2

[Botond Ballo [:botond]]

Sure! Thanks for your interest.

The first thing to do is to set up a Firefox build, as described at https://developer.mozilla.org/en-US/docs/Simple_Firefox_build.

If you've done that successfully, you're ready to get started. 

I suggest fixing this bug in stages. In the first stage, let's do this:

  - Change the type of Layer::mVisibleRegion to be LayerIntRegion.
    Layer is defined in gfx/layers/Layers.h.

  - Go through the uses of mVisibleRegion. You can use the DXR
    code browser tool to find these, see [1].

    For each use:

      - If the code is _reading_ the value of mVisbleRegion and
        expecting an nsIntRegion, change mVisibleRegion to
        mVisibleRegion.ToUnknownRegion(). ToUnknownRegion() is
        a function defined by all typed regions (e.g. LayerIntRegions)
        that converts them to an untyped region (nsIntRegion).

      - If the code is _setting_ the value of mVisibleRegion and
        providing an nsIntRegion, wrap the nsIntRegion value being
        provided with LayerIntRegion::FromUnknownRegion(), which
        converts the nsIntRegion to a LayerIntRegion.

In further stages, we can propagate the type change to some call sites, like the getter and setter, and LayerComposite::mVisibleRegion, but let's not worry about that for now.

I've assigned the bug to you. If you have any questions or run into any issues, please feel free to ask! You can ask by commenting in this bug, or you can find me on IRC - my nick is 'botond', and I hang out in the channels #apz and #gfx.

[1] https://dxr.mozilla.org/mozilla-central/search?q=%2Bvar-ref%3Amozilla%3A%3Alayers%3A%3ALayer%3A%3AmVisibleRegion

Comment 3

[Sunny Sidhu]

Thanks Botond,

I've started this but I think I'm wrong! Please see the changes I tried as per (my interpretation of) your instructions:

>  // Layers.h
>  From:
>  
>  nsIntRegion mVisibleRegion;
>  
>  To:
>  
>  LayerIntRegion mVisibleRegion;

and

>  // Layers.h
>  From:
>  
>  virtual void SetVisibleRegion(const nsIntRegion& aRegion)
>  {
>    if (!mVisibleRegion.IsEqual(aRegion)) {
>      MOZ_LAYERS_LOG_IF_SHADOWABLE(this, ("Layer::Mutated(%p) VisibleRegion was %s is %s", this,
>        mVisibleRegion.ToString().get(), aRegion.ToString().get()));
>      mVisibleRegion = aRegion;
>      Mutated();
>    }
>  }
>  
>  To:
>
>  virtual void SetVisibleRegion(const nsIntRegion& aRegion)
>  {
>    if (!mVisibleRegion.ToUnknownRegion().IsEqual(aRegion)) {
>      MOZ_LAYERS_LOG_IF_SHADOWABLE(this, ("Layer::Mutated(%p) VisibleRegion was %s is %s", this,
>        mVisibleRegion.ToUnknownRegion().ToString().get(), aRegion.ToString().get()));
>      mVisibleRegion = LayerIntRegion::FromUnknownRegion(aRegion);
>      Mutated();
>    }
>  }

Could you indicate what I'm doing wrong?

Comment 4

[Botond Ballo [:botond]]

That looks fine to me. What makes you say it's wrong?

The only change I would suggest is leaving mVisibleRegion.ToString() as-is, since ToString() returns the same result regardless of the type of the region.

Note that once we propagate the change to the signature of SetVisibleRegion() (in a later patch), this code will become neater.

Comment 5

[Sunny Sidhu]

Attachment: rev1 - Handles read and set values of mVisibleRegion

Thanks for the feedback. I guess the compiler errors .

I've applied the changes now I think, ignoring the "getter" instances. Please can you provide feedback on my patch.

Comment 6

[Botond Ballo [:botond]]

Comment on attachment 8689787 [details] [diff] [review]
rev1 - Handles read and set values of mVisibleRegion

Review of attachment 8689787 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, this is a good start! 

There are a couple of places you are missing:

  - Layer::GetVisibleRegion()
  - Layer::GetIntermediateSurfaceRect()

::: gfx/layers/Layers.h
@@ +846,5 @@
>     * of its visible region, it needs to ensure that what it draws is valid.
>     */
>    virtual void SetVisibleRegion(const nsIntRegion& aRegion)
>    {
>      if (!mVisibleRegion.IsEqual(aRegion)) {

You need to convert either mVisibleRegion or aRegion here.

(In my previous comment, I was referring to mVisibleRegion.ToString() only.)

Comment 7

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #6)
>   - Layer::GetVisibleRegion()

I realized there is a problem with this. GetVisibleRegion() returns the visible region by reference, so if we have to convert from another type, we'd be returning a reference to a temporary.

For now we can work around this by changing GetVisibleRegion() to return by value, but once we propagate the type change to the signature of GetVisibleRegion(), we should change it back to return by reference.

Comment 8

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #7)
> For now we can work around this by changing GetVisibleRegion() to return by
> value

In turn, the same thing needs to be done for Layer::GetEffectiveVisibleRegion().

Comment 9

[Sunny Sidhu]

Attachment: rev2 - Handles read and set values of mVisibleRegion

Thanks for the feedback!

Please find my revised patch attached.

Comment 10

[Botond Ballo [:botond]]

Comment on attachment 8690041 [details] [diff] [review]
rev2 - Handles read and set values of mVisibleRegion

Review of attachment 8690041 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, Sunny, this looks good! Just one small comment:

::: gfx/layers/Layers.h
@@ +1456,5 @@
>    // These getters can be used anytime.  They return the effective
>    // values that should be used when drawing this layer to screen,
>    // accounting for this layer possibly being a shadow.
>    const Maybe<ParentLayerIntRect>& GetEffectiveClipRect();
> +  const nsIntRegion GetEffectiveVisibleRegion();

You need to make this change to the implementation of the function in Layers.cpp, too.

Comment 11

[Botond Ballo [:botond]]

Oh, there is one other place that needs adjustment: on this line [1], since the type of mVisibleRegion has changed, its GetBounds().Size() now evaluates to a LayerIntSize instead of an IntSize, so there is a type mismatch.

[1] https://dxr.mozilla.org/mozilla-central/source/gfx/layers/client/ClientTiledPaintedLayer.cpp?from=ClientTiledPaintedLayer.cpp#413

Comment 12

[Botond Ballo [:botond]]

After fixing the two issues above, I think you're ready to go on to the next stage of this bug, which is to propagate the type change to the getter and setter of Layer::mVisibleRegion, and make adjustments to _their_ call sites as necessary.

That is, the argument type of SetVisibleRegion() and the return type of GetVisibleRegion() should be changed from nsIntRegion to LayerIntRegion. In doing so, the conversions we had to add to the implementations of these functions in the first stage, can be removed. We can also go back to having GetVisibleRegion() (and GetEffectiveVisibleRegion()) return by reference, as we would no longer be returning a reference to a temporary now that we're not converting the type.

Comment 13

[Sunny Sidhu]

Attachment: rev3 - Propagating type changes

Thanks for the fast feedback Botond.

The attached patch isn't the completed version and I realise there may be some errors. I feel this would be a good stage for some feedback if that's ok.

I had a couple of questions also:

1) In Layers.h on line 1631 there is a call to GetVisibleRegion, with the result getting stored in a variable called mInvalidRegion. Presumably I need to also change the type of mInvalidRegion in its definition? If so, do I also propagate the changes to other references for mInvalidRegion, like on line 1623 for the method GetInvalidRegion?

2) Applying a similar kind of context to the above, as I was changing GetEffectiveVisibleRegion to return by reference I wondered if I need to change its type to LayerIntRegion as well?

Comment 14

[Botond Ballo [:botond]]

Comment on attachment 8690767 [details] [diff] [review]
rev3 - Propagating type changes

Review of attachment 8690767 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! This generally looks good; I have a few comments.

::: dom/base/nsDOMWindowUtils.cpp
@@ +2571,5 @@
>          return false;
>        child = child->GetNextSibling();
>      }
>    } else {
> +    LayerIntRegion rgn = aLayer->GetVisibleRegion();

This isn't going to compile without further changes, because things like 'offset' and 'aCoveredRegion' are still untyped. Rather than converting all of them, I suggest we convert here, i.e. keep 'rgn' untyped and initialize it with 'aLayer->GetVisibleRegion().ToUnknownRegion()'.

::: gfx/layers/LayerTreeInvalidation.cpp
@@ +252,5 @@
>  
>    RefPtr<Layer> mLayer;
>    UniquePtr<LayerPropertiesBase> mMaskLayer;
>    nsTArray<UniquePtr<LayerPropertiesBase>> mAncestorMaskLayers;
> +  LayerIntRegion mVisibleRegion;

You're going to need to change a few use sites of this variable; compiler errors you get in this file should guide you.

::: gfx/layers/Layers.cpp
@@ +617,5 @@
>    }
>    return GetClipRect();
>  }
>  
> +const LayerIntRegion

We can go back to returning by reference here.

::: gfx/layers/RotatedBuffer.cpp
@@ +446,5 @@
>      canUseOpaqueSurface ? gfxContentType::COLOR :
>                            gfxContentType::COLOR_ALPHA;
>  
>    SurfaceMode mode;
> +  LayerIntRegion neededRegion;

Let's keep this one untyped (and convert at the point where it's assigned from aLayer->GetVisibleRegion()), as many other things would have to be changed in this file to convert it.

::: gfx/layers/composite/ContainerLayerComposite.cpp
@@ +82,5 @@
>  
>    std::stringstream ss;
>    aLayer->PrintInfo(ss, "");
>  
> +  LayerIntRegion visibleRegion = aLayer->GetVisibleRegion();

You're going to get a type mismatch a few lines below where we assign visibleRegion.GetBounds().TopLeft(); best convert here.

::: gfx/tests/gtest/TestLayers.cpp
@@ +181,5 @@
>  }
>  
>  already_AddRefed<Layer> CreateLayerTree(
>      const char* aLayerTreeDescription,
> +    LayerIntRegion* aVisibleRegions,

You'll have to add a ToUnknownRegion() in one place below where we use this to initialize an EventRegions object.

::: layout/base/FrameLayerBuilder.cpp
@@ +2414,5 @@
>   * own coordinate system to which the layer's visible region is restricted.
>   * Consumes *aOuterVisibleRegion.
>   */
>  static void
> +SetOuterVisibleRegion(Layer* aLayer, LayerIntRegion* aOuterVisibleRegion,

It's probably better to keep this an nsIntRegion for now, and just convert when we call SetVisibleRegion() at the end of the function; otherwise, many things in this function and its callers would have to be converted.

Comment 15

[Botond Ballo [:botond]]

(In reply to Sunny Sidhu from comment #13)
> 1) In Layers.h on line 1631 there is a call to GetVisibleRegion, with the
> result getting stored in a variable called mInvalidRegion. Presumably I need
> to also change the type of mInvalidRegion in its definition? If so, do I
> also propagate the changes to other references for mInvalidRegion, like on
> line 1623 for the method GetInvalidRegion?
> 
> 2) Applying a similar kind of context to the above, as I was changing
> GetEffectiveVisibleRegion to return by reference I wondered if I need to
> change its type to LayerIntRegion as well?

Good questions!

We have to draw a line somewhere between what we convert and what we don't, otherwise we can easily spend weeks converting code to typed units. I suggest that in this bug, we convert the "visible region" and things immediately related to it (like the "effective visible region" and "shadow visible region"), but not other things (like the "valid region"). We can convert other things like the "valid region" in future bugs.

So, my suggested answers are:
   (1) Keep mInvalidRegion untyped for now.
   (2) Change GetEffectiveVisibleRegion() to return LayerIntRegion.

Comment 16

[Sunny Sidhu]

Attachment: rev4 - Propagating type changes

Thanks for the feedback, very helpful!

Please find my revised patch attached.

Comment 17

[Botond Ballo [:botond]]

Comment on attachment 8691435 [details] [diff] [review]
rev4 - Propagating type changes

Review of attachment 8691435 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, looks like you're getting the hang of this :)

There are still several places where you need to add ToUnknownRegion() or FromUnknownRegion() calls. If you try building with this patch, the compiler will point them out to you.

One caution: in a few places, the result of GetVisibleRegion() or GetEffectiveVisibleRegion() is assigned to a local variable of type |const nsIntRegion&|. Your options are, as usual:

  (1) Change the local variable to be of type |const LayerIntRegion&|, and 
      adjust its uses accordingly.

  (2) Add a ToUnknownRegion() at the end of the initializer.

If you do (2), it's important to also change the type of the variable to not be a reference, otherwise the reference would be referring to a temporary.

::: gfx/layers/LayerTreeInvalidation.cpp
@@ +80,5 @@
>    return intRect;
>  }
>  
>  static void
> +AddTransformedRegion(LayerIntRegion& aDest, const LayerIntRegion& aSource, const Matrix4x4& aTransform)

This doesn't need to change - the call sites still pass in nsIntRegions.

::: gfx/layers/Layers.h
@@ +1314,5 @@
>     *  transform is found to be non-translational. This method returns early
>     *  in this case, results will not be valid. Returns true on successful
>     *  traversal.
>     */
> +  bool GetVisibleRegionRelativeToRootLayer(LayerIntRegion& aResult,

Let's keep this an nsIntRegion for now. Otherwise, we need to deal with the uses of aResult in the function body, and doing that properly is tricky.

::: gfx/layers/RotatedBuffer.cpp
@@ +453,5 @@
>    bool canReuseBuffer = HaveBuffer();
>  
>    while (true) {
>      mode = aLayer->GetSurfaceMode();
> +    neededRegion = aLayer->GetVisibleRegion.ToUnknownRegion();

Missing a () here.

Comment 18

[Sunny Sidhu]

Attachment: rev5-compilerError271115.PNG

Hi Botond,

I've been trying to get through the changes. 

I've hit a stumbling block with a compiler error and was wondering if you could help? Please find a screenshot attached. The latest revision of my patch follows this message.

Comment 19

[Sunny Sidhu]

Attachment: rev5 - Propagating type changes

Comment 20

[Botond Ballo [:botond]]

(In reply to Sunny Sidhu from comment #18)
> I've hit a stumbling block with a compiler error and was wondering if you
> could help? Please find a screenshot attached.

The problem here is that the function ThebesRect() expects an untyped rect (IntRect), but since we've changed GetEffectiveVisibleRegion() to return a typed region, the arguments being passed (e.g. aOne->GetEffectiveVisibleRegion().GetBounds()) are typed rects (LayerIntRect), so there is a type mismatch.

The solution is similar to other places: convert the effective visible region in the argument to untyped using ToUnknownRegion().

Comment 21

[Botond Ballo [:botond]]

Comment on attachment 8693034 [details] [diff] [review]
rev5 - Propagating type changes

Review of attachment 8693034 [details] [diff] [review]:
-----------------------------------------------------------------

Getting there :) There are still some compiler errors left to fix besides the one you mentioned in comment 18.

::: gfx/layers/LayerTreeInvalidation.cpp
@@ +80,5 @@
>    return intRect;
>  }
>  
>  static void
> +AddTransformedRegion(nsIntRegion& aDest, const nsIntRegion aSource, const Matrix4x4& aTransform)

You removed a reference here.

::: gfx/layers/Layers.cpp
@@ +1012,5 @@
>    return quad;
>  }
>  
>  bool
> +Layer::GetVisibleRegionRelativeToRootLayer(nsIntRegion aResult,

You removed a reference here, too.

::: gfx/layers/Layers.h
@@ +1330,5 @@
>     *  transform is found to be non-translational. This method returns early
>     *  in this case, results will not be valid. Returns true on successful
>     *  traversal.
>     */
> +  bool GetVisibleRegionRelativeToRootLayer(nsIntRegion aResult,

Removed a reference

::: layout/base/FrameLayerBuilder.cpp
@@ +5742,5 @@
>                                        Layer* aLayer, nscolor
>                                        aBackgroundColor)
>  {
>    if (NS_GET_A(aBackgroundColor) > 0) {
> +    nsIntRect r = aLayer->GetVisibleRegion().ToUnknownRegion().GetBounds();

Here, we might as well change r to have type LayerIntRect instead.

Comment 22

[Sunny Sidhu]

Attachment: rev6 - Propagating type changes

Thanks. :)

Please find attached my new patch, this time it builds!

Comment 23

[Botond Ballo [:botond]]

Comment on attachment 8693127 [details] [diff] [review]
rev6 - Propagating type changes

Review of attachment 8693127 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, Sunny, this looks great!

There is one other place I think we should change before the patch is ready to land:

Layers are transferred from the main thread to the compositor thread using an inter-process communication mechanism called IPDL. The representation of the data structures that carry the layer attributes "over the wire" are defined using a domain-specific language associated with IPDL. In this representation, the visible region is still stored as an nsIntRegion [1]. I'd like to change that to be LayerIntRegion as well.

To do this:

  1) Import the type LayerIntRegion by adding a line similar to these [2] at the top of 
     LayersMessages.ipdhl.

  2) Change the declared type at [1].

  3) The conversions added to ShadowLayers.cpp (which populates the over-the-wire 
     representation) and LayerTransactionParent.cpp (which reads the value out of the
     over-the-wire representation) can be removed.

[1] https://dxr.mozilla.org/mozilla-central/rev/47b49b0d32360fab04b11ff9120970979c426911/gfx/layers/ipc/LayersMessages.ipdlh#205
[2] https://dxr.mozilla.org/mozilla-central/rev/47b49b0d32360fab04b11ff9120970979c426911/gfx/layers/ipc/LayersMessages.ipdlh#36

::: gfx/layers/apz/test/gtest/TestAsyncPanZoomController.cpp
@@ +2754,5 @@
>      SetDefaultAllowedTouchBehavior(manager, blockId);
>    }
>    mcc->AdvanceByMillis(100);
>  
> +  layers[0]->SetVisibleRegion(LayerIntRegion::FromUnknownRegion(nsIntRegion(IntRect(0,50,200,150))));

Here, let's just construct the correct types to begin with:

  layers[0]->SetVisibleRegion(LayerIntRegion(LayerIntRect(0,50,200,150)));

::: layout/base/FrameLayerBuilder.cpp
@@ +3180,5 @@
>        // Hide the PaintedLayer. We leave it in the layer tree so that we
>        // can find and recycle it later.
>        ParentLayerIntRect emptyRect;
>        data->mLayer->SetClipRect(Some(emptyRect));
> +      data->mLayer->SetVisibleRegion(LayerIntRegion::FromUnknownRegion(nsIntRegion()));

Just construct a LayerIntRegion:

  data->mLayer->SetVisibleRegion(LayerIntRegion());

Comment 24

[Botond Ballo [:botond]]

I also pushed the current patch to the Try server to see if there are any uses in platform-specific code that need to be converted:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c8e22368e1c0

Comment 25

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #24)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=c8e22368e1c0

It looks like there is at least one place in B2G-specific (in widget/gonk/HwcComposer2D.cpp) that needs to be updated.

Comment 26

[Sunny Sidhu]

Attachment: rev7 - Propagating type changes

Thanks Botond. I've tried to apply the remaining updates but have come across a compiler error that, after investigating, I'm not sure how to deal with. Please see the screenshot in the following comment.

Comment 27

[Sunny Sidhu]

Attachment: rev7-compiler-error.PNG

This comes as a result of the ipc changes.

Comment 28

[Botond Ballo [:botond]]

(In reply to Sunny Sidhu from comment #26)
> I've tried to apply the remaining updates but have come
> across a compiler error that, after investigating, I'm not sure how to deal
> with. Please see the screenshot in the following comment.

Ah, I forgot that one more step is required.

In order to use a type in the over-the-wire representation, we need to define how to serialize and deserialize that type. This is done by specializing a template class named ParamTraits for the type in question, and defining in it Write() and Read() methods that do the serialization and deserialization, respectively. 

Currently, we have specialized ParamTraits for nsIntRegion [1], but not for typed regions. As a result, we are getting an error message saying that ParamTraits<LayerIntRegion>::Write() and Read() are not defined.

To fix this, we can modify our specialization of ParamTraits to cover all region types. Note that nsIntRegion is a typedef for IntRegionTyped<UnknownUnits>, and LayerIntRegion is a typedef for IntRegionTyped<LayerPixel>. We can write a specialization of ParamTraits that covers all variants of IntRegionTyped. Conveniently, we already have a helper class RegionParamTraits that does most of the work for us, so our modified specialization will be fairly simple. It would look something like this:

  template<class Units>
  struct ParamTraits<IntRegionTyped<Units>>
    : RegionParamTraits<IntRegionTyped<Units>,
                        IntRectTyped<Units>,
                        typename IntRegionTyped<Units>::RectIterator>
  {};

I haven't tested this (not in front of my dev machine right now), but it should work. Give it a try and let me know how it goes!

[1] https://dxr.mozilla.org/mozilla-central/rev/7883e81f3c305078353ca27a6b1adb8c769d5904/gfx/ipc/GfxMessageUtils.h#400

Comment 29

[Sunny Sidhu]

Attachment: compilerError.PNG

Thanks for the quick response.

I've tried this, the compiler doesn't seem to like the code. Am I correct in assuming that I can remove the specialized ParamTraits for nsIntRegion? That's what I did before replacing it with this code.

Please see the attached screenshot for the errors.

Comment 30

[Botond Ballo [:botond]]

(In reply to Sunny Sidhu from comment #29)
> I've tried this, the compiler doesn't seem to like the code. 

The problem here is that the types IntRegionTyped and IntRectTyped are in the namespace mozilla::gfx, but ParamTraits is in a different namespace (IPC), so we need to use the fully qualified names:

  template<class Units>
  struct ParamTraits<mozilla::gfx::IntRegionTyped<Units>>
    : RegionParamTraits<mozilla::gfx::IntRegionTyped<Units>,
                        mozilla::gfx::IntRectTyped<Units>,
                        typename mozilla::gfx::IntRegionTyped<Units>::RectIterator>
  {};


> Am I correct in
> assuming that I can remove the specialized ParamTraits for nsIntRegion?
> That's what I did before replacing it with this code.

Yup. The new specialization covers nsIntRegion, because it's a typedef for IntRegionTyped<UnknownUnits>.

Comment 31

[Sunny Sidhu]

Attachment: rev8 - Propagates type changes

Ah, works! Thanks for all the guidance Botond. :)

Please find the patch attached.

Comment 32

[Sunny Sidhu]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=87abfe9c3589

Comment 33

[Botond Ballo [:botond]]

(In reply to Sunny Sidhu from comment #32)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=87abfe9c3589

Thanks for doing a Try push :) It looks clean!

Comment 34

[Botond Ballo [:botond]]

Comment on attachment 8693284 [details] [diff] [review]
rev8 - Propagates type changes

Review of attachment 8693284 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

Comment 35

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/294b89a6fdae

Comment 36

[Botond Ballo [:botond]]

Rebased the patch against bug 1208829 and landed on mozilla-inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/294b89a6fdae

Comment 37

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/294b89a6fdae

Comment 38

[Botond Ballo [:botond]]

Thanks a lot for your work on this, Sunny!

If you're interested in working on more unit changes, or other things, let me know and I'm happy to suggest something.

Comment 39

[Sunny Sidhu]

It was fun and thank you also for your excellent support. :)

Thanks, I may be in touch soon!