You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@flex.apache.org by mi...@apache.org on 2014/10/22 17:29:00 UTC
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
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.
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...@apache.org>.
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)
> {
>
>