Need your support: source code review of StringColourItem.java (JavaME)

[Gert]

DictionaryForMIDs JavaME uses the class StringColourItem for displaying the coloured translation texts (by default the coloured display is activated in the settings form). StringColourItem an important class in DictionaryForMIDs. When StringColourItem was introduced in DictionaryForMIDs version 3, this was a big improvement in the "look&feel" of DictionaryForMIDs !

However, the code of the class StringColourItem is hard to read, and it turned out that in some cases the implementation shows 'incorrect behaviour' (aka bugs).

For this reason we are asking for support of a code review of StringColourItem. Who can help here ?

Here are the tasks:

• review the code to check whether there is an incorrect implementation (bug)
  
• 
  check whether the code makes correct use of the MIDP API (StringColourItem is a MIDP CustomItem class)
• 
  if possible: amend the source code with comments (currently hardly any comments are found in the source)
  
• 
  of course corrections for detected problems will be welcome too
  

Priority 1 is review of the code that is executed by the method pointerPressed. Review of the remaining code is priority 2.

Here is a link to the source of StringColourItem: http://dictionarymid.svn.sourceforge.net/viewvc/dictionarymid/trunk/JavaME/src/de/kugihan/dictionaryformids/hmi_java_me/lcdui_extension/StringColourItem.java?view=markup

Note: StringColourItem supports 'selections' of text (see also 'selectionMode' in the source). Currently those selections are decativated in de.kugihan.dictionaryformids.dataaccess.content.PredefinedContent by using selectionModeNone. For this reason also the triggering of translations by pointing on the touchscreen' (results in a call to pointerPressed) is deactivated. I still will provide a test version where PredefinedContent has its selection modes set to selectionModeSingle.

Thanks in advance !
Gert

[Gert]

I just created the branch JavaME_3_3_1_StringColourItem_branch which we can use for debugging/improving the StringColourItem class (I did put both the JavaME and the DictionaryForMIDs paths in that branch).

In that branch I modified PredefinedContent so that all predefined contents have a selection mode of selectionModeSingle. I built a version with dictionary from that source which can be downloaded here:
http://www.kugihan.de/dict/download/test_versions/3.3_StringColourItemTest/DictionaryForMIDs_EngTagSpa_CAL.jad
http://www.kugihan.de/dict/download/test_versions/3.3_StringColourItemTest/DictionaryForMIDs_EngTagSpa_CAL.jar

And here the 'empty versions':
http://www.kugihan.de/dict/download/test_versions/3.3_StringColourItemTest/DictionaryForMIDs.jad
http://www.kugihan.de/dict/download/test_versions/3.3_StringColourItemTest/DictionaryForMIDs.jar

This version may help you in testing.

Best regards,
Gert

[NewAgeMobile]

My SE K800i has problems with the formatting with the English-Chinese dictionary.
I guessed that this was something to do with most of the characters being undefined, and breaking Font.stringWidth.
I tried a workaround to replace that function, which seems to be ok (well, a whole lot better on it in any case).

Code Select Expand


private int strWidthWorkaround(Font f, String s) {
 if (s == null || s.length() == 0)
     return 1;
 int w = 0;
 for (int i = 0; i < s.length(); ++i) {
  int wi = f.charWidth(s.charAt(i));
  if (wi <= 0 || wi >= 16)
      wi = f.charWidth('w');
  w += wi;
 }
 return w;
}


EDIT: the wi>=16 was just for the test I did. Perhaps use: wi>=f.charWidth('w') instead.

There is perhaps little point since a load of 'squares' in both the Chinese and Pinyin is hardly a translation worth using^^
Sadly there are, by the looks of it, similar problems with the BitmapFont. (I will need to set up on device debugging to get somewhere with this one, and dunno if I can be bothered).

I also think that for niceness, this (or similar) should be included:

Code Select Expand


 protected void sizeChanged(int w,int h) {
   setWidth(w);
 }


btw: The SE workaround in the main form is not needed on this device. In fact it is a problem to have on it. I wonder if a simple Thread.yield() (to allow a repaint) works for those that do need that workaround.

[Gert]

Now I am impressed ... you appear 'out of the nothing' and you present us corrections for the cumbersome StringColourItem !!! If we only had a lot more people like you on the project   

For the slow people - like me - let me try to repeat if I well understood:

1.

I guessed that this was something to do with most of the characters being undefined, and breaking Font.stringWidth.
I tried a workaround to replace that function, which seems to be ok (well, a whole lot better on it in any case).

Code:

Code Select Expand

private int strWidthWorkaround(Font f, String s) {
  if (s == null || s.length() == 0)
      return 1;
  int w = 0;
  for (int i = 0; i < s.length(); ++i) {
   int wi = f.charWidth(s.charAt(i));
   if (wi <= 0 || wi >= 16)
       wi = f.charWidth('w');
   w += wi;
  }
  return w;
}


EDIT: the wi>=16 was just for the test I did. Perhaps use: wi>=f.charWidth('w') instead.

That part is the improvement for devices that do not have a complete Chinese font; your workaround will solve formatting problems, but of course the non-available font characters will still not be shown. Did I get this right ?

Besides, did you replace the plenty occurrences of Font.stringWidth with your workaround for testing ?


2.

There is perhaps little point since a load of 'squares' in both the Chinese and Pinyin is hardly a translation worth using^^
Sadly there are, by the looks of it, similar problems with the BitmapFont. (I will need to set up on device debugging to get somewhere with this one, and dunno if I can be bothered).

Ah, this is crucial: also the BitmapFont seem to exhibit problems there, right ?!?
What means "if I can be bothered" ?


3.

I also think that for niceness, this (or similar) should be included:

Code:

Code Select Expand

  protected void sizeChanged(int w,int h) {
    setWidth(w);
  }


I deactivated sizeChanged in version 3.4 cause it was throwing Exceptions on some devices. Of course, if after some testing your proposal will work fine, I will be glad to incorporate it for 3.5 !


4.

btw: The SE workaround in the main form is not needed on this device. In fact it is a problem to have on it. I wonder if a simple Thread.yield() (to allow a repaint) works for those that do need that workaround.

That SE workaround(s) caused us a lot of nightmares; it was pure 'try and error'; and it seems that depending on the SE Java Platform and Java Platform revision number the bug shows up at a code location. I mean the bug within the SE Java implementation.

Well, maybe a simple Thread.yield() may have a good effect; we really would need to have someone with an affected device in order to test.

In fact it is a problem to have on it.

Well, what is that problem ? Guess I forgot about this ...

Thank you a lot !!!
Gert

[NewAgeMobile]

Besides, did you replace the plenty occurrences of Font.stringWidth with your workaround for testing ?

There are only about 4 calls in the class.
EDIT: missing thos outside of the formatting for the item size^^

Better not get too excited. Alas I have problems testing the Chinese version with ODD, so I cannot be sure if that 'apparent fix' is THE fix and I may have jumped the gun with it.
I will do some more ODD now I have set it up and try see for sure what was happening. I can use the English-Khmer ok with ODD and this has non-standard characters.

I deactivated sizeChanged in version 3.4 cause it was throwing Exceptions on some devices. Of course, if after some testing your proposal will work fine, I will be glad to incorporate it for 3.5 !

Really? what type of exception? The device would call this when the size changes ie. when the scrollbar is added as things become to tall for fitting on the screen all at once.

I am a bit confused here though; if the item size changes, you should call invalidate() to notify the device, but lol. the j2me docs say this can then result in a call to sizeChanged. Hmm. ok, maybe an

if(oldHeight!=newHeight etc.)
invalidate()

Would be needed to stop a nasty loop with that one.

That SE workaround(s) caused us a lot of nightmares; it was pure 'try and error'; and it seems that depending on the SE Java Platform and Java Platform revision number the bug shows up at a code location. I mean the bug within the SE Java implementation.

Well, maybe a simple Thread.yield() may have a good effect; we really would need to have someone with an affected device in order to test.

Agreed, an affected device is needed for the test.

In fact it is a problem to have on it.

Well, what is that problem ? Guess I forgot about this ...

hehe. it causes the items not to show; results in the reason for the workaround.

[Gert]

If you could make some progress with the on-device-debugger, that would be great !!

Really? what type of exception?

It was the old code within sizeChanged; it made a call back to MainForm, which then did thrown the exception.

hehe. it causes the items not to show; results in the reason for the workaround.

Oh ... well,  if we only knew about the root of the problems in the SE Java implementation, then we might be able to find a working workaround.

Thanks !
Gert

[NewAgeMobile]

The SE appears not to need this Font.stringwidth() workaround after all. Further tests have shown that I got fooled by the 'nothing showing up' problem being worse on some word translations than others, sometimes intermittent, and strangely often it is the same Items that will not get painted 

Items that are added to the form, but are not visible without scrolling are being set to an odd height with my SE; the display height. And this is why I thought there was a problem being caused by the undefined characters and Font.StringWidth() .

This seems to be fixable by forcing the VM to reset the layout using a call to invalidate(), though I need to do more work and testing to find out where is best. Right now putting it in showNotify() seems to work best.

Oh ... well,  if we only knew about the root of the problems in the SE Java implementation, then we might be able to find a working workaround.

I think that although some of the changes are going to be relevant to StringColourItem, it will take this thread more and more off topic to talk about the SE here. So I will move to a new one when I have more and post a link to that here.

The sizeChanged that I provided before may still be good to keep (testing is needed on other devices), when the VM adds a scrollbar, it would make sense to recalculate things, as a new width may cause the word wrapping to change and thus alter the height needed.

Tom

[Gert]

Ok, thank you for keeping us updated with your progress !!

Concerning the SE topic of your postings, we simply can split that to a new thread ("Split Topic", next to Quote, Modify, Remove). Hmmm, maybe you cannot, cause you do not have the right to do so ? If not, then tell me.

Besides, I assume that the formatting problems of your SE model only show up for the 'Coloured display' (which uses StringColourItem). If you switch off  'Coloured display', then I assume the output will not look nice (cause of the missing colouring), but the formatting will be ok, is this right ?

Best regards,
Gert

[NewAgeMobile]

Concerning the SE topic of your postings, we simply can split that to a new thread ("Split Topic", next to Quote, Modify, Remove). Hmmm, maybe you cannot, cause you do not have the right to do so ? If not, then tell me.

Yep I do not have the rights.

Besides, I assume that the formatting problems of your SE model only show up for the 'Coloured display' (which uses StringColourItem). If you switch off  'Coloured display', then I assume the output will not look nice (cause of the missing colouring), but the formatting will be ok, is this right ?

Yes the formatting is ok without. The fact is that really it is the bitmap font that I need to get working since I have no Chinese characters and it was this dictionary that bought me to your app.
The problems are related; it is the way the SE VM is handling customItem.

Before I split/move threads. Umm a potential indexOutOfBounds in StringColourItem or just paranoia?

Code Select Expand


    public void clearSelectedWord() {
            //if (currSelectItem >= 0 && currSelectItem < itemTextWrap.length)  //un-comment and add this!
            itemTextWrap[currSelectItem] = ClearSelectChar(itemTextWrap[currSelectItem]);
            selectedWord = null;
            currSelectItem = -1;
            repaint();
    }



[Gert]

Will be great if you manage to get something fixed for the SE BitmapFont handling !

Concerning clearSelectedWord: honestly speaking I do not fully understand the code; in any case, adding your proposed if-statement will not harm. Just collect all your improvement proposals, so that those can be incorporated in a test version.

Regards,
Gert

[NewAgeMobile]

My findings RE the SonyEricsson (KVM) bug:
http://dictionarymid.sourceforge.net/forum/index.php?topic=172.msg859#msg859

Great. now I can do something else