Bug 104225 - [CellEditors] Using Viewer for ComboBoxCellEditor
Summary: [CellEditors] Using Viewer for ComboBoxCellEditor
Status: VERIFIED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 3.1   Edit
Hardware: PC Windows XP
: P3 enhancement with 3 votes (vote)
Target Milestone: 3.4 M4   Edit
Assignee: Thomas Schindl CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on: 69254
Blocks:
  Show dependency tree
 
Reported: 2005-07-18 11:05 EDT by Thomas Schindl CLA
Modified: 2008-04-30 10:05 EDT (History)
8 users (show)

See Also:
tom.schindl: review? (bokowski)


Attachments
ComboBox-Editor using CComboViewer (534 bytes, text/plain)
2005-07-18 11:07 EDT, Thomas Schindl CLA
no flags Details
Viewer based upon CCombo instead of Combo (2.35 KB, text/plain)
2005-07-18 11:08 EDT, Thomas Schindl CLA
no flags Details
The cell editor based upon a viewer (7.73 KB, patch)
2006-12-13 08:46 EST, Thomas Schindl CLA
no flags Details | Diff
A snippet if we add this class (5.24 KB, text/plain)
2007-05-09 11:45 EDT, Thomas Schindl CLA
no flags Details
Combo (17.64 KB, image/png)
2007-12-12 14:45 EST, Kim Horne CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Thomas Schindl CLA 2005-07-18 11:05:32 EDT
it would be really great if we could use the same MVC for ComboBoxes in tables
like we do with normal ComboBoxes
Comment 1 Thomas Schindl CLA 2005-07-18 11:07:17 EDT
Created attachment 24911 [details]
ComboBox-Editor using CComboViewer
Comment 2 Thomas Schindl CLA 2005-07-18 11:08:01 EDT
Created attachment 24912 [details]
Viewer based upon CCombo instead of Combo
Comment 3 Eric Moffatt CLA 2005-08-15 10:19:06 EDT
A quick look reveals that the current implementation of the ComboboxCellEditor 
now uses CCombo as its base control....
Comment 4 Thomas Schindl CLA 2005-08-16 04:00:11 EDT
(In reply to comment #3)
> A quick look reveals that the current implementation of the ComboboxCellEditor 
> now uses CCombo as its base control....
> 
I'm not sure what you are trying to say with this comment. 
Yes ComboboxCellEditor uses CCombo because Combo could not be resized on all
platforms as far as I know. 
The 2 files only implement a viewer based on it so one can use the normal viewer
programming when using TableViewer instead of a different implementation when
dealing with tables. I've written this viewer for a datasource-binding framework
I'm creating using viewers I can use the same logic in Tables-Forms than I do I
normal Forms.

Comment 5 Thomas Schindl CLA 2006-10-30 06:01:48 EST
Eric, Tod, Boris what do you think about providing Viewer-based ComboBoxCellEditor?
Comment 6 Thomas Schindl CLA 2006-12-13 08:46:41 EST
Created attachment 55579 [details]
The cell editor based upon a viewer

Now that ComboViewer can deal with CCombo it would be a great thing to use that in Table/Trees
Comment 7 Thomas Schindl CLA 2007-05-09 07:25:21 EDT
Eric would you mind passing over to me I'd like to include this in 3.4 time
Comment 8 Eric Moffatt CLA 2007-05-09 10:10:58 EDT
I have no problem with hadning off my defects...;-)

Done! Thanks Tom
Comment 9 Thomas Schindl CLA 2007-05-09 11:45:02 EDT
Created attachment 66503 [details]
A snippet if we add this class
Comment 10 Eric Rizzo CLA 2007-11-05 15:43:35 EST
Is there anything in the attached cell editor that would prevent its use in Eclipse 3.3?
Comment 11 Thomas Schindl CLA 2007-11-05 15:44:44 EST
no
Comment 12 Thomas Schindl CLA 2007-11-07 12:26:33 EST
Boris this is new API so you'll have to +1 or request me to change the API which IMHO is fairly straight foward. I'd like to get this out of my M4 queue ASAP
Comment 13 Boris Bokowski CLA 2007-11-07 16:43:32 EST
Looks good except for this:

import java.text.MessageFormat; // Not using ICU to support standalone JFace
// scenario

This was likely copied from somewhere else, so I cannot blame you ;-).  The problem is that java.text is not part of CDC Foundation 1.0 AFAIK.
Comment 14 Thomas Schindl CLA 2007-11-07 17:31:34 EST
(In reply to comment #13)
> Looks good except for this:
> 
> import java.text.MessageFormat; // Not using ICU to support standalone JFace
> // scenario
> 
> This was likely copied from somewhere else, so I cannot blame you ;-).  The
> problem is that java.text is not part of CDC Foundation 1.0 AFAIK.
> 

Yes from ComboBoxCellEditor :-)
Comment 15 Thomas Schindl CLA 2007-11-07 17:43:47 EST
And what has to be used instead, we needed. 

The following Classes make use of MessageFormat in JFace:
- ComboBoxCellEditor
- DialogCellEditor
- TextCellEditor
- ExternalActionManager
- JFaceResources
Comment 16 Thomas Schindl CLA 2007-11-07 17:56:46 EST
Released to CVS-HEAD > 20071107 I'm logging a seperate bug where we are going to fix the MessageFormat thingie
Comment 17 Thomas Schindl CLA 2007-11-07 18:00:26 EST
marking fixed the MessageFormat problem is tracked in bug #209108
Comment 18 Kim Horne CLA 2007-12-12 14:44:04 EST
Verified that it works on OS X but should it really be this ugly?  screencap coming
Comment 19 Kim Horne CLA 2007-12-12 14:45:07 EST
Created attachment 85111 [details]
Combo
Comment 20 Thomas Schindl CLA 2007-12-12 14:50:37 EST
the width of the second column should be adjusted but this is the is the same with ComboBoxCellEditor
Comment 21 Eric Rizzo CLA 2007-12-17 11:19:01 EST
(In reply to comment #10)
> Is there anything in the attached cell editor that would prevent its use in
> Eclipse 3.3?
> 

I'm attempting to use this code in a 3.3-based plugin, but have a couple of small issues that I'm not sure are bugs or just due to subtle incompatibilities with 3.3

Looking at doSetValue(Object value), shouldn't it handle the case when the value is null, like this:

protected void doSetValue(Object value) {
    Assert.isTrue(viewer != null);
    if (value == null) {
        viewer.setSelection(null);
    } else {
        viewer.setSelection(new StructuredSelection(value));
    }
}

Also, the selectedValue is not updated as part of doSetValue() which seems to lead to a bug (at least on 3.3) where if the cell is activated via mouse click and then immediately tabbed out (without interacting with the CCombo), the cell editor returns null to doGetValue(). It seems it should be fixed by setting selectedValue in doSetValue(), like this:

protected void doSetValue(Object value) {
    Assert.isTrue(viewer != null);
    selectedvalue = value;
    if (value == null) {
        viewer.setSelection(null);
    } else {
        viewer.setSelection(new StructuredSelection(value));
    }
}

Are these legitimate bugs or just a function of trying to adapt this to 3.3?
Comment 22 Thomas Schindl CLA 2007-12-17 16:44:52 EST
Thanks Eric, those look like legitimate bugs. Could you file a new bug, attach a patch and I'll happily integrate it.
Comment 23 Eric Rizzo CLA 2007-12-18 09:51:59 EST
(In reply to comment #22)
> Thanks Eric, those look like legitimate bugs. Could you file a new bug, attach
> a patch and I'll happily integrate it.
> 

Done, Bug 213315
Comment 24 Thomas Schindl CLA 2008-04-30 10:05:09 EDT
verified that the new class is in in I20080430