[patch] fix bug 4612

[patch] fix bug 4612

[Zhao Shujing]

[-- Attachment #1: Type: text/plain, Size: 258 bytes --]

Hi, 
Please take a look at the attached patch.
It is to fix bug 4612.
It also does:
- Make the column display of memory window synchronous with the  fromBox
when adjust the fromSpin.
- Do the same to disassembly window when adjust the toSpin.

Thanks
Pearly

[-- Attachment #2: Type: text/x-patch, Size: 5105 bytes --]

diff -ur frysk-cvs/frysk-gui/frysk/gui/ChangeLog frysk/frysk-gui/frysk/gui/ChangeLog
--- frysk-cvs/frysk-gui/frysk/gui/ChangeLog	2007-07-26 10:11:33.000000000 -0400
+++ frysk/frysk-gui/frysk/gui/ChangeLog	2007-07-26 10:17:20.000000000 -0400
@@ -1,3 +1,13 @@
+2007-07-23  Zhao Shujing  <pearly.zhao@oracle.com>
+
+	* memory/MemoryWindow.java: fromBox and toBox can be edited. Fixes
+	#4612.
+	* memory/MemoryWindow.java(handleFromSpin): synchronously display
+	column and fromBox when adjust the fromSpin.
+	* disassembler/DisassemblyWindow.java: fromBox and toBox can be edited.
+	* disassembler/DisassemblyWindow.java(rowAppend): synchronously display
+	column and toBox when adjust the toSpin.
+
 2007-07-19  Adam Jocksch  <ajocksch@redhat.com>
 
 	* sessions/WatchListListener.java: Reformatted.
diff -ur frysk-cvs/frysk-gui/frysk/gui/disassembler/DisassemblyWindow.java frysk/frysk-gui/frysk/gui/disassembler/DisassemblyWindow.java
--- frysk-cvs/frysk-gui/frysk/gui/disassembler/DisassemblyWindow.java	2007-07-26 10:11:33.000000000 -0400
+++ frysk/frysk-gui/frysk/gui/disassembler/DisassemblyWindow.java	2007-07-26 10:14:53.000000000 -0400
@@ -345,12 +345,12 @@
     {
       public void entryEvent (EntryEvent arg0)
       {
-        if (arg0.getType() == EntryEvent.Type.CHANGED)
+        if (arg0.getType() == EntryEvent.Type.ACTIVATE)
           {
             if (refreshLock)
               return;
           
-            String str = arg0.getText();
+            String str = fromBox.getText();
             try
             {
               double d = (double) Long.parseLong(str, 16);
@@ -368,12 +368,12 @@
     {
       public void entryEvent (EntryEvent arg0)
       {
-        if (arg0.getType() == EntryEvent.Type.CHANGED)
+        if (arg0.getType() == EntryEvent.Type.ACTIVATE)
           {
             if (refreshLock)
               return;
             
-              String str = arg0.getText();
+              String str = toBox.getText();             
               try
               {
                 double d = (double) Long.parseLong(str, 16);
@@ -592,6 +592,14 @@
         this.lastPath.next();
         if (ins != null)
           {
+            if (li.hasNext())
+                ins = (Instruction) li.next();
+              else
+                {
+                  this.toSpin.setValue((double) ins.address);
+                  this.lastKnownTo = ins.address;
+                  ins = null;
+                }
             model.setValue(iter, (DataColumnString) cols[1],
                            "<pc+" + (ins.address - this.pc) + ">: ");
             model.setValue(iter, (DataColumnString) cols[LOC],
@@ -599,14 +607,6 @@
             model.setValue(iter, (DataColumnString) cols[2], ins.instruction);
             model.setValue(iter, (DataColumnObject) cols[3], ins);
             
-            if (li.hasNext())
-              ins = (Instruction) li.next();
-            else
-              {
-                this.toSpin.setValue((double) ins.address);
-                this.lastKnownTo = ins.address;
-                ins = null;
-              }
           }
         else
           model.setValue(iter, (DataColumnString) cols[1], "");
@@ -679,9 +679,10 @@
           }
       }
     
-    refreshList();
+    this.fromSpin.setValue(val);
     this.lastKnownFrom = val;
     this.fromBox.setText(Long.toHexString((long) val));
+    refreshList();
   }
 
   boolean toToggle = false;
diff -ur frysk-cvs/frysk-gui/frysk/gui/memory/MemoryWindow.java frysk/frysk-gui/frysk/gui/memory/MemoryWindow.java
--- frysk-cvs/frysk-gui/frysk/gui/memory/MemoryWindow.java	2007-07-26 10:11:33.000000000 -0400
+++ frysk/frysk-gui/frysk/gui/memory/MemoryWindow.java	2007-07-26 10:14:45.000000000 -0400
@@ -475,12 +475,12 @@
     {
       public void entryEvent (EntryEvent arg0)
       {
-        if (arg0.getType() == EntryEvent.Type.CHANGED)
+        if (arg0.getType() == EntryEvent.Type.ACTIVATE)
           {
             if (refreshLock)
               return;
             
-            String str = arg0.getText();
+            String str = fromBox.getText();
             try
             {
               double d = (double) Long.parseLong(str, 16);
@@ -499,12 +499,12 @@
     {
       public void entryEvent (EntryEvent arg0)
       {
-        if (arg0.getType() == EntryEvent.Type.CHANGED)
+        if (arg0.getType() == EntryEvent.Type.ACTIVATE)
           {
             if (refreshLock)
               return;
             
-              String str = arg0.getText();
+              String str = toBox.getText();
               try
               {
                 double d = (double) Long.parseLong(str, 16);
@@ -874,7 +874,7 @@
       {
         TreeIter iter = model.getFirstIter();
 
-        for (int i = (int) lastKnownFrom; i < (int) val; i++)
+        for (long i = (long) lastKnownFrom; i < (long) val; i++)
           {
             model.removeRow(iter);
             iter = iter.getNextIter();
@@ -931,6 +931,7 @@
       }
 
     this.toBox.setText(Long.toHexString((long) val));
+    this.toSpin.setValue(val);
     refreshList();
   }
 

Re: [patch] fix bug 4612

[Andrew Cagney]

Pearly,

The below looks fine.  Will you be around tuesday morning (your time?).  
I'll send you off list some stuff to do with CVS so that you can check 
this in.

Two things to think about though:

-> testing;
When making fixes we all endeavor to author the test-case up-front so 
that we can directly demonstrate that the change has the intended effect.

-> it looks like there is much code duplication between the Disassembler 
and Memory windows;
an opportunity to refactor?

Andrew

Zhao Shujing wrote:
> Hi, 
> Please take a look at the attached patch.
> It is to fix bug 4612.
> It also does:
> - Make the column display of memory window synchronous with the  fromBox
> when adjust the fromSpin.
> - Do the same to disassembly window when adjust the toSpin.
>
> Thanks
> Pearly
>   
> ------------------------------------------------------------------------
>
> diff -ur frysk-cvs/frysk-gui/frysk/gui/ChangeLog frysk/frysk-gui/frysk/gui/ChangeLog
> --- frysk-cvs/frysk-gui/frysk/gui/ChangeLog	2007-07-26 10:11:33.000000000 -0400
> +++ frysk/frysk-gui/frysk/gui/ChangeLog	2007-07-26 10:17:20.000000000 -0400
> @@ -1,3 +1,13 @@
> +2007-07-23  Zhao Shujing  <pearly.zhao@oracle.com>
> +
> +	* memory/MemoryWindow.java: fromBox and toBox can be edited. Fixes
> +	#4612.
> +	* memory/MemoryWindow.java(handleFromSpin): synchronously display
> +	column and fromBox when adjust the fromSpin.
> +	* disassembler/DisassemblyWindow.java: fromBox and toBox can be edited.
> +	* disassembler/DisassemblyWindow.java(rowAppend): synchronously display
> +	column and toBox when adjust the toSpin.
> +
>  2007-07-19  Adam Jocksch  <ajocksch@redhat.com>
>  
>  	* sessions/WatchListListener.java: Reformatted.
> diff -ur frysk-cvs/frysk-gui/frysk/gui/disassembler/DisassemblyWindow.java frysk/frysk-gui/frysk/gui/disassembler/DisassemblyWindow.java
> --- frysk-cvs/frysk-gui/frysk/gui/disassembler/DisassemblyWindow.java	2007-07-26 10:11:33.000000000 -0400
> +++ frysk/frysk-gui/frysk/gui/disassembler/DisassemblyWindow.java	2007-07-26 10:14:53.000000000 -0400
> @@ -345,12 +345,12 @@
>      {
>        public void entryEvent (EntryEvent arg0)
>        {
> -        if (arg0.getType() == EntryEvent.Type.CHANGED)
> +        if (arg0.getType() == EntryEvent.Type.ACTIVATE)
>            {
>              if (refreshLock)
>                return;
>            
> -            String str = arg0.getText();
> +            String str = fromBox.getText();
>              try
>              {
>                double d = (double) Long.parseLong(str, 16);
> @@ -368,12 +368,12 @@
>      {
>        public void entryEvent (EntryEvent arg0)
>        {
> -        if (arg0.getType() == EntryEvent.Type.CHANGED)
> +        if (arg0.getType() == EntryEvent.Type.ACTIVATE)
>            {
>              if (refreshLock)
>                return;
>              
> -              String str = arg0.getText();
> +              String str = toBox.getText();             
>                try
>                {
>                  double d = (double) Long.parseLong(str, 16);
> @@ -592,6 +592,14 @@
>          this.lastPath.next();
>          if (ins != null)
>            {
> +            if (li.hasNext())
> +                ins = (Instruction) li.next();
> +              else
> +                {
> +                  this.toSpin.setValue((double) ins.address);
> +                  this.lastKnownTo = ins.address;
> +                  ins = null;
> +                }
>              model.setValue(iter, (DataColumnString) cols[1],
>                             "<pc+" + (ins.address - this.pc) + ">: ");
>              model.setValue(iter, (DataColumnString) cols[LOC],
> @@ -599,14 +607,6 @@
>              model.setValue(iter, (DataColumnString) cols[2], ins.instruction);
>              model.setValue(iter, (DataColumnObject) cols[3], ins);
>              
> -            if (li.hasNext())
> -              ins = (Instruction) li.next();
> -            else
> -              {
> -                this.toSpin.setValue((double) ins.address);
> -                this.lastKnownTo = ins.address;
> -                ins = null;
> -              }
>            }
>          else
>            model.setValue(iter, (DataColumnString) cols[1], "");
> @@ -679,9 +679,10 @@
>            }
>        }
>      
> -    refreshList();
> +    this.fromSpin.setValue(val);
>      this.lastKnownFrom = val;
>      this.fromBox.setText(Long.toHexString((long) val));
> +    refreshList();
>    }
>  
>    boolean toToggle = false;
> diff -ur frysk-cvs/frysk-gui/frysk/gui/memory/MemoryWindow.java frysk/frysk-gui/frysk/gui/memory/MemoryWindow.java
> --- frysk-cvs/frysk-gui/frysk/gui/memory/MemoryWindow.java	2007-07-26 10:11:33.000000000 -0400
> +++ frysk/frysk-gui/frysk/gui/memory/MemoryWindow.java	2007-07-26 10:14:45.000000000 -0400
> @@ -475,12 +475,12 @@
>      {
>        public void entryEvent (EntryEvent arg0)
>        {
> -        if (arg0.getType() == EntryEvent.Type.CHANGED)
> +        if (arg0.getType() == EntryEvent.Type.ACTIVATE)
>            {
>              if (refreshLock)
>                return;
>              
> -            String str = arg0.getText();
> +            String str = fromBox.getText();
>              try
>              {
>                double d = (double) Long.parseLong(str, 16);
> @@ -499,12 +499,12 @@
>      {
>        public void entryEvent (EntryEvent arg0)
>        {
> -        if (arg0.getType() == EntryEvent.Type.CHANGED)
> +        if (arg0.getType() == EntryEvent.Type.ACTIVATE)
>            {
>              if (refreshLock)
>                return;
>              
> -              String str = arg0.getText();
> +              String str = toBox.getText();
>                try
>                {
>                  double d = (double) Long.parseLong(str, 16);
> @@ -874,7 +874,7 @@
>        {
>          TreeIter iter = model.getFirstIter();
>  
> -        for (int i = (int) lastKnownFrom; i < (int) val; i++)
> +        for (long i = (long) lastKnownFrom; i < (long) val; i++)
>            {
>              model.removeRow(iter);
>              iter = iter.getNextIter();
> @@ -931,6 +931,7 @@
>        }
>  
>      this.toBox.setText(Long.toHexString((long) val));
> +    this.toSpin.setValue(val);
>      refreshList();
>    }
>  
>   

Re: [patch] fix bug 4612

[Kris Van Hees]

The code duplication between the Disassembler and Memory windows has been
mentioned during the conf call where I pointed out the various problems
found with those windows (and their implementation).  It certainly could do
with a refactoring, although it is too early for that right now.  For one,
I do not think it is wise to do it as part of a bug fix, and secondly, the
planned changes to the Memory window will cause it to deviate more from the
Disassembler window than it does now, so premature refactoring is likely to
need a (partial) reversal later on.

	Cheers,
	Kris

On Mon, Jul 30, 2007 at 09:02:19AM -0400, Andrew Cagney wrote:
> Pearly,
>
> The below looks fine.  Will you be around tuesday morning (your time?).  
> I'll send you off list some stuff to do with CVS so that you can check this 
> in.
>
> Two things to think about though:
>
> -> testing;
> When making fixes we all endeavor to author the test-case up-front so that 
> we can directly demonstrate that the change has the intended effect.
>
> -> it looks like there is much code duplication between the Disassembler 
> and Memory windows;
> an opportunity to refactor?

Re: [patch] fix bug 4612

[Kris Van Hees]

Woops, my apologies for sending this off before responding to the first point
as well (I guess I do need more coffee):

I do agree that testing is important (obviously), and hope that we will be able
to increase our GUI test coverage as time goes by.  However, it clearly has
been a problem in the past as well (as witnessed by the very limited amount of
GUI tests that currently exist), and that is very understandable given the
framework needed to create those tests.  Andrew even mentioned at OLS thatthe
GUI is essentially not tested.

But we do need to get these bugs fixed, and make the GUI usable.  Given the
complexity writing GUI tests (using dogtail), it seem more prudent to wait for
the affected windows to stabilize to write tests.  Otherwise it's just going
to be a wasted effort.

	Cheers,
	Kris

On Mon, Jul 30, 2007 at 10:39:27AM -0400, Kris Van Hees wrote:
> The code duplication between the Disassembler and Memory windows has been
> mentioned during the conf call where I pointed out the various problems
> found with those windows (and their implementation).  It certainly could do
> with a refactoring, although it is too early for that right now.  For one,
> I do not think it is wise to do it as part of a bug fix, and secondly, the
> planned changes to the Memory window will cause it to deviate more from the
> Disassembler window than it does now, so premature refactoring is likely to
> need a (partial) reversal later on.
> 
> 	Cheers,
> 	Kris
> 
> On Mon, Jul 30, 2007 at 09:02:19AM -0400, Andrew Cagney wrote:
> > Pearly,
> >
> > The below looks fine.  Will you be around tuesday morning (your time?).  
> > I'll send you off list some stuff to do with CVS so that you can check this 
> > in.
> >
> > Two things to think about though:
> >
> > -> testing;
> > When making fixes we all endeavor to author the test-case up-front so that 
> > we can directly demonstrate that the change has the intended effect.
> >
> > -> it looks like there is much code duplication between the Disassembler 
> > and Memory windows;
> > an opportunity to refactor?

Re: [patch] fix bug 4612

[Andrew Cagney]

Kris,

Our guideline is: re-factor early, re-factor often(1).  Here we trust 
and allow Pearly to make a decision such as when to best apply a 
refactorings.

Andrew

(1) A refactoring , by definition, is not part of a functional change 
such as a bug fix.

Kris Van Hees wrote:
> The code duplication between the Disassembler and Memory windows has been
> mentioned during the conf call where I pointed out the various problems
> found with those windows (and their implementation).  It certainly could do
> with a refactoring, although it is too early for that right now.  For one,
> I do not think it is wise to do it as part of a bug fix, and secondly, the
> planned changes to the Memory window will cause it to deviate more from the
> Disassembler window than it does now, so premature refactoring is likely to
> need a (partial) reversal later on.
>
> 	Cheers,
> 	Kris
>
> On Mon, Jul 30, 2007 at 09:02:19AM -0400, Andrew Cagney wrote:
>   
>> Pearly,
>>
>> The below looks fine.  Will you be around tuesday morning (your time?).  
>> I'll send you off list some stuff to do with CVS so that you can check this 
>> in.
>>
>> Two things to think about though:
>>
>> -> testing;
>> When making fixes we all endeavor to author the test-case up-front so that 
>> we can directly demonstrate that the change has the intended effect.
>>
>> -> it looks like there is much code duplication between the Disassembler 
>> and Memory windows;
>> an opportunity to refactor?
>>     

Re: [patch] fix bug 4612

[Andrew Cagney]

Kris,

Yes, the dogtail testing.  You'll see that there frysk-gui tree now also 
contains JUnit tests; and we're able to more directly test components 
using that.

Pearly, you may want to talk to Sami about opportunities here.

Andrew


Kris Van Hees wrote:
> Woops, my apologies for sending this off before responding to the first point
> as well (I guess I do need more coffee):
>
> I do agree that testing is important (obviously), and hope that we will be able
> to increase our GUI test coverage as time goes by.  However, it clearly has
> been a problem in the past as well (as witnessed by the very limited amount of
> GUI tests that currently exist), and that is very understandable given the
> framework needed to create those tests.  Andrew even mentioned at OLS thatthe
> GUI is essentially not tested.
>
> But we do need to get these bugs fixed, and make the GUI usable.  Given the
> complexity writing GUI tests (using dogtail), it seem more prudent to wait for
> the affected windows to stabilize to write tests.  Otherwise it's just going
> to be a wasted effort.
>
> 	Cheers,
> 	Kris
>
> On Mon, Jul 30, 2007 at 10:39:27AM -0400, Kris Van Hees wrote:
>   
>> The code duplication between the Disassembler and Memory windows has been
>> mentioned during the conf call where I pointed out the various problems
>> found with those windows (and their implementation).  It certainly could do
>> with a refactoring, although it is too early for that right now.  For one,
>> I do not think it is wise to do it as part of a bug fix, and secondly, the
>> planned changes to the Memory window will cause it to deviate more from the
>> Disassembler window than it does now, so premature refactoring is likely to
>> need a (partial) reversal later on.
>>
>> 	Cheers,
>> 	Kris
>>
>> On Mon, Jul 30, 2007 at 09:02:19AM -0400, Andrew Cagney wrote:
>>     
>>> Pearly,
>>>
>>> The below looks fine.  Will you be around tuesday morning (your time?).  
>>> I'll send you off list some stuff to do with CVS so that you can check this 
>>> in.
>>>
>>> Two things to think about though:
>>>
>>> -> testing;
>>> When making fixes we all endeavor to author the test-case up-front so that 
>>> we can directly demonstrate that the change has the intended effect.
>>>
>>> -> it looks like there is much code duplication between the Disassembler 
>>> and Memory windows;
>>> an opportunity to refactor?
>>>       

Re: [patch] fix bug 4612

[Sami Wagiaalla]

Andrew Cagney wrote:
> Kris,
>
> Yes, the dogtail testing.  You'll see that there frysk-gui tree now 
> also contains JUnit tests; and we're able to more directly test 
> components using that.
>
> Pearly, you may want to talk to Sami about opportunities here.
Data structures, and objects that the gui relies on, such as Observers, 
Sessions are tested through junit. This is the thin layer under the gui 
and ontop of the core.

So you might not be able to test that a button does what it should do, 
but the code it relies on is junit testable.

Sami

Re: [patch] fix bug 4612

[Sami Wagiaalla]

Andrew Cagney wrote:
> Kris,
>
> Yes, the dogtail testing.  You'll see that there frysk-gui tree now 
> also contains JUnit tests; and we're able to more directly test 
> components using that.
>
> Pearly, you may want to talk to Sami about opportunities here.
By the way phil is a good reference too, he wrote more than half of 
those tests and is intimately familiar with the gui code.

Re: [patch] fix bug 4612

[Elena Zannoni]

To clarify:
This patch was discussed on IRC and Mark and other folks have provided
useful feedback and testing.  The patch had also been tested by Kris before
being posted by Pearly. Kris has already committed Pearly's patch,
since nobody but him and Pearly are concentrating on that part
of GUI right now.

An ack to the mailing list could have been sent, since discussing things
on IRC exclusively is not really optimal practice for a fully open
source project. I'd like to take the opportunity to appeal to the
wider group to be more consistent in doing so.

Kris committing this patch has aroused an unfortunate not so friendly
private reaction, which I will not post here. What I'd like to bring up here
though is about approval of patches. It is my personal belief for it to
be good practice to not be anointed with write access just because one
happens to be tasked to work on the project. I have asked Pearly to submit
a few patches before applying for write access to the repository. Since
the group has been widening, and hopefully it will widen more as
Frysk matures nicely, there are cases in which people w/o commit rights
will want to post patches occasionally (we have seen this already with
Tom or others). So far people who felt comfortable taking responsibility
for such patches have committed them. I do believe this is a good way
to incorporate contributions from outside the committers list.

Re: [patch] fix bug 4612

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 2190 bytes --]

Hi Elena,

On Mon, 2007-07-30 at 13:26 -0400, Elena Zannoni wrote:
> To clarify:
> This patch was discussed on IRC and Mark and other folks have provided
> useful feedback and testing.  The patch had also been tested by Kris before
> being posted by Pearly. Kris has already committed Pearly's patch,
> since nobody but him and Pearly are concentrating on that part
> of GUI right now.
> 
> An ack to the mailing list could have been sent, since discussing things
> on IRC exclusively is not really optimal practice for a fully open
> source project. I'd like to take the opportunity to appeal to the
> wider group to be more consistent in doing so.

That seems like a good policy. I will also try to summarize and post
more about anything on the list that was discussed on irc to keep an
archive of stuff for people to look at. We seem pretty scattered around
according to timezones over the whole planet so there is always someone
missing out on some discussion somewhere.

> Kris committing this patch has aroused an unfortunate not so friendly
> private reaction, which I will not post here. What I'd like to bring up here
> though is about approval of patches. It is my personal belief for it to
> be good practice to not be anointed with write access just because one
> happens to be tasked to work on the project. I have asked Pearly to submit
> a few patches before applying for write access to the repository. Since
> the group has been widening, and hopefully it will widen more as
> Frysk matures nicely, there are cases in which people w/o commit rights
> will want to post patches occasionally (we have seen this already with
> Tom or others). So far people who felt comfortable taking responsibility
> for such patches have committed them. I do believe this is a good way
> to incorporate contributions from outside the committers list.

Yes, I am more than happy to take responsibility for reviewing and
committing peoples patches if they ask. In this case I thought the patch
was good but wanted to have a second opinion from Rick and Kris before
committing because they are much more familiar with gui code than me.

Cheers,

Mark

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]