You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@flex.apache.org by Mihai Chira <mi...@apache.org> on 2014/10/22 17:40:31 UTC
Re: git commit: [flex-tlf] [refs/heads/develop] - FLEX-26478 CAUSE:
When replacing ContainerControllers (triggered when the text changes in a
TextField / TextArea), StandardFlowComposer tried to dispose of the discarded
ones first. It did this by calling cle
Piotr or Alex, could you (or someone else familiar with TLF) please take a
look at my commit? Let me know if you would have solved (or diagnosed) it
differently. Also, do you have an idea how we could create a unit /
mustella test for this bug?
PS: when I ran the unit tests, ContainerAttributeTest.paddingBottomTest was
failing both before and after my commit, with this error: "Paragraph3's top
value should be 87.55. - expected true but was false"
On 22 October 2014 16:29, <mi...@apache.org> wrote:
> Repository: flex-tlf
> Updated Branches:
> refs/heads/develop 129ec697e -> d1c83617f
>
>
> FLEX-26478
> CAUSE:
> When replacing ContainerControllers (triggered when the text changes in a
> TextField / TextArea), StandardFlowComposer tried to dispose of the
> discarded ones first. It did this by calling clearSelectionShapes() and
> setRootElement(null) on the ContainerControllers. However, if that
> controller was already listening for mouse scroll events (when the user
> selects text and moves the mouse outside the text field in such a way that
> causes the text to scroll), this listener wasn't removed. Which meant that
> the next time scrollTimerHandler() was called (on the inactive
> ContainerController), textFlow was null due to the call to
> setRootElement(null).
>
> SOLUTION:
> StandardFlowComposer now calls a new method 'discard' on the
> ContainerControllers which are not needed anymore. This method now also
> stops the selection scroll timer ('_scrollTimer').
>
>
> Project: http://git-wip-us.apache.org/repos/asf/flex-tlf/repo
> Commit: http://git-wip-us.apache.org/repos/asf/flex-tlf/commit/d1c83617
> Tree: http://git-wip-us.apache.org/repos/asf/flex-tlf/tree/d1c83617
> Diff: http://git-wip-us.apache.org/repos/asf/flex-tlf/diff/d1c83617
>
> Branch: refs/heads/develop
> Commit: d1c83617f4a32b4c39417ed53dd26c5aa2df7503
> Parents: 129ec69
> Author: Mihai Chira <mi...@apache.org>
> Authored: Wed Oct 22 16:23:40 2014 +0100
> Committer: Mihai Chira <mi...@apache.org>
> Committed: Wed Oct 22 16:23:40 2014 +0100
>
> ----------------------------------------------------------------------
> .../textLayout/compose/StandardFlowComposer.as | 3 +-
> .../textLayout/container/ContainerController.as | 35 +++++++++++++++-----
> 2 files changed, 27 insertions(+), 11 deletions(-)
> ----------------------------------------------------------------------
>
>
>
> http://git-wip-us.apache.org/repos/asf/flex-tlf/blob/d1c83617/textLayout/src/flashx/textLayout/compose/StandardFlowComposer.as
> ----------------------------------------------------------------------
> diff --git
> a/textLayout/src/flashx/textLayout/compose/StandardFlowComposer.as
> b/textLayout/src/flashx/textLayout/compose/StandardFlowComposer.as
> index cb0b38e..3393d32 100644
> --- a/textLayout/src/flashx/textLayout/compose/StandardFlowComposer.as
> +++ b/textLayout/src/flashx/textLayout/compose/StandardFlowComposer.as
> @@ -185,8 +185,7 @@ package flashx.textLayout.compose
> var cont:ContainerController;
> for each (cont in _controllerList)
> {
> - cont.clearSelectionShapes();
> - cont.setRootElement(null);
> + cont.dispose();
> }
> }
>
>
>
> http://git-wip-us.apache.org/repos/asf/flex-tlf/blob/d1c83617/textLayout/src/flashx/textLayout/container/ContainerController.as
> ----------------------------------------------------------------------
> diff --git
> a/textLayout/src/flashx/textLayout/container/ContainerController.as
> b/textLayout/src/flashx/textLayout/container/ContainerController.as
> index 4876884..de77fa7 100644
> --- a/textLayout/src/flashx/textLayout/container/ContainerController.as
> +++ b/textLayout/src/flashx/textLayout/container/ContainerController.as
> @@ -27,6 +27,7 @@ package flashx.textLayout.container
> import flash.events.ContextMenuEvent;
> import flash.events.Event;
> import flash.events.FocusEvent;
> + import flash.events.IEventDispatcher;
> import flash.events.IMEEvent;
> import flash.events.KeyboardEvent;
> import flash.events.MouseEvent;
> @@ -1770,6 +1771,29 @@ package flashx.textLayout.container
> return createDefaultContextMenu();
> }
>
> + public function dispose():void
> + {
> + stopMouseSelectionScrolling();
> + clearSelectionShapes();
> + setRootElement(null);
> + }
> +
> + private function
> stopMouseSelectionScrolling(containerRoot:IEventDispatcher = null):void
> + {
> + if(_scrollTimer)
> + {
> + _scrollTimer.stop();
> + _scrollTimer.removeEventListener(TimerEvent.TIMER,
> scrollTimerHandler);
> +
> + if(!containerRoot)
> + containerRoot = getContainerRoot();
> +
> + if(containerRoot)
> +
> containerRoot.removeEventListener(MouseEvent.MOUSE_UP, scrollTimerHandler);
> +
> + _scrollTimer = null;
> + }
> + }
> /** @private */
> tlf_internal function scrollTimerHandler(event:Event):void
> {
> @@ -1785,19 +1809,12 @@ package flashx.textLayout.container
> // We're listening for MOUSE_UP so we can cancel
> autoscrolling
> if (event is MouseEvent)
> {
> - _scrollTimer.stop();
> -
> _scrollTimer.removeEventListener(TimerEvent.TIMER, scrollTimerHandler);
> +
> stopMouseSelectionScrolling(event.currentTarget as IEventDispatcher);
> CONFIG::debug { assert(_container.stage
> == null || getContainerRoot() == event.currentTarget,"scrollTimerHandler
> bad target"); }
> -
> event.currentTarget.removeEventListener(MouseEvent.MOUSE_UP,
> scrollTimerHandler);
> - _scrollTimer = null;
> }
> else if (!event)
> {
> - _scrollTimer.stop();
> -
> _scrollTimer.removeEventListener(TimerEvent.TIMER, scrollTimerHandler);
> - if (getContainerRoot())
> -
> getContainerRoot().removeEventListener( MouseEvent.MOUSE_UP,
> scrollTimerHandler);
> - _scrollTimer = null;
> + stopMouseSelectionScrolling();
> }
> else if (_container.stage)
> {
>
>
Re: git commit: [flex-tlf] [refs/heads/develop] - FLEX-26478 CAUSE:
When replacing ContainerControllers (triggered when the text changes in a
TextField / TextArea), StandardFlowComposer tried to dispose of the
discarded ones first. It did this by calling cle
Posted by piotrz <pi...@gmail.com>.
Great Mihai!
Sorry about p. 2 I didn't catch finally block. :) You are right!
Unfortunately I don't know to much about mustella tests...
Thanks,
Piotr
-----
Apache Flex PMC
piotrzarzycki21@gmail.com
--
View this message in context: http://apache-flex-development.2333347.n4.nabble.com/Re-git-commit-flex-tlf-refs-heads-develop-FLEX-26478-CAUSE-When-replacing-ContainerControllers-trigge-tp41683p41701.html
Sent from the Apache Flex Development mailing list archive at Nabble.com.
Re: git commit: [flex-tlf] [refs/heads/develop] - FLEX-26478 CAUSE:
When replacing ContainerControllers (triggered when the text changes in a
TextField / TextArea), StandardFlowComposer tried to dispose of the discarded
ones first. It did this by calling cle
Posted by Mihai Chira <mi...@gmail.com>.
Thank you for the review, Piotr.
1) Done, thanks for noticing.
2) scrollTimerHandler() only nulls out the _scrollTimer temporarily,
and then puts it back in the finally clause. It looks like even if
there's an error thrown in interactionManager.mouseMoveHandler, the
finally clause will still run and re-assign the _scrollTimer:
"_scrollTimer = stashedScrollTimer;" Given all this, I don't think
it's a good idea to create a separate function just for nulling the
_scrollTimer. Do you?
3) also made some small improvements (like optimising imports) in
StandardFlowComposer and ContainerController.
Finally, any thoughts about the unit/mustella test we could build around this?
Thanks,
Mihai
On 23 October 2014 06:31, piotrz <pi...@gmail.com> wrote:
> Hi Mihai,
>
> It looks really good for me! I am also not an expert in TLF, but what you
> did looks reasonable.
> I have two comments:
>
> 1) In your function stopMouseSelectionScrolling I would use for condition
> curly brackets.
> Why? I think it's more readable, and such place is become less I would say
> expose on error.
> I saw so many times in my inspected code that devs did mistakes with
> condition without curly brackets.
> Ex:
>
> If (something)
> oldcode;
> my new code should be inside condition but I forgot to add curly brackets;
>
> I hope you understand what I mean.
>
> 2) In your function you are removing _scrollTimer and it's good, but we have
> such removes also in method scrollTimerHandler, but without removing
> registered event.
>
> Let's create a function for removing this scroll timer and use this function
> inside stopMouseSelectionScrolling and scrollTimerHandler.
>
> This function should remove registered event listener and nulling our timer.
>
> Thanks,
> Piotr
>
>
>
>
>
> -----
> Apache Flex PMC
> piotrzarzycki21@gmail.com
> --
> View this message in context: http://apache-flex-development.2333347.n4.nabble.com/Re-git-commit-flex-tlf-refs-heads-develop-FLEX-26478-CAUSE-When-replacing-ContainerControllers-trigge-tp41683p41696.html
> Sent from the Apache Flex Development mailing list archive at Nabble.com.
Re: git commit: [flex-tlf] [refs/heads/develop] - FLEX-26478 CAUSE:
When replacing ContainerControllers (triggered when the text changes in a
TextField / TextArea), StandardFlowComposer tried to dispose of the
discarded ones first. It did this by calling cle
Posted by piotrz <pi...@gmail.com>.
Hi Mihai,
It looks really good for me! I am also not an expert in TLF, but what you
did looks reasonable.
I have two comments:
1) In your function stopMouseSelectionScrolling I would use for condition
curly brackets.
Why? I think it's more readable, and such place is become less I would say
expose on error.
I saw so many times in my inspected code that devs did mistakes with
condition without curly brackets.
Ex:
If (something)
oldcode;
my new code should be inside condition but I forgot to add curly brackets;
I hope you understand what I mean.
2) In your function you are removing _scrollTimer and it's good, but we have
such removes also in method scrollTimerHandler, but without removing
registered event.
Let's create a function for removing this scroll timer and use this function
inside stopMouseSelectionScrolling and scrollTimerHandler.
This function should remove registered event listener and nulling our timer.
Thanks,
Piotr
-----
Apache Flex PMC
piotrzarzycki21@gmail.com
--
View this message in context: http://apache-flex-development.2333347.n4.nabble.com/Re-git-commit-flex-tlf-refs-heads-develop-FLEX-26478-CAUSE-When-replacing-ContainerControllers-trigge-tp41683p41696.html
Sent from the Apache Flex Development mailing list archive at Nabble.com.