Community
Participate
Working Groups
Created attachment 97201 [details] example code 20080422-0800 Not sure if it is a bug or intended: in the measure event (SWT.MeasureItem), it's currently not possible to find out if the element is selected: SWT.SELECTED is never set in event.detail. It would be nice to have this information so we don't need to do extra work in paint (want to prepare the text layout once in measure (needed for the width) and just apply it in paint) See the example snippet. Sorry for not realizing this earlier.
This was intended (but I can't think of a reason why this information should not be included). Probably won't get to this for 3.4, unless critical. The measure event was originally indended to configure the scroll bars and column widths for pack(). Then is was extended to be used for hittesting, hover and selection drawing. Note that it is sent on hover but erase and paint may not be sent later. This happens when the mouse moves out of the hover rectangle before the operating system hover conditions are satisfied. Is this a problem?
Our implementation of the StyledCellLabelProvider now constructs the text layout for selected items two times: - Once in measure to compute the width - Once in paint, same as before but without colors If we knew the selection state in 'measure', we could do it right the first time. But that's only for the selected items. So its not critical. But nice to have.
This would still be nice to have. It doesn't sound hard to fix, and it would save us unnecessary TextLayout#setStyle(..) and subsequent computeRuns(..) for each selected item. (In reply to comment #1) > Note that it [measure event] is sent on hover but erase and paint > may not be sent later. This happens when the mouse moves out of the hover > rectangle before the operating system hover conditions are satisfied. Is this > a problem? No, as long as measure is always sent before erase and paint (bug 228695), that's no problem.
Any chance to get this fixed for 3.7?
Created attachment 180463 [details] Patch for Win32
Created attachment 180464 [details] Patch for Mac/Cocoa
Cocoa and Win32 changes attached. I don't have a GTK install handy, but the changes will be much smaller.
--- IMO about the code I verifed the code in Win32, it looks good. Usually send(Measure/Erase/Paint)ItemEvent does all the work internally to compute the detail field. We could change Tree#sendMeasureItemEvent() to follow the same design by calling isItemSelected (item.handle, gc.handle, 0) in the method. But the current code is fine as it reuses the is select state in two places. I guess the only change I'd suggest is to change in the new parameter in sendMeasureItemEvent () from isSelected to detail, this way it will require less code change in the future if we need to grow more information on the event. --- IMO about the idea If we add the selection status to the detail field for MeasureItem event now we will open the door for people to ask the detail field to include all other status (as detail field during MeasureItem would be inconsistent if compared to PaintItem, EraseItem events). Martin, can you change your code from: boolean isSelected= (event.detail & SWT.SELECTED) != 0; // doesn't work at the moment: see bug. to: Table table = item.getParent(); boolean isSelected= table.isSelected(indexOf(item)); That is basically what Scott's patch is doing for Table anyway...
(In reply to comment #8) > If we add the selection status to the detail field for MeasureItem event now we > will open the door for people to ask the detail field to include all other > status (as detail field during MeasureItem would be inconsistent if compared to > PaintItem, EraseItem events). Don't worry, people will ask even if you don't open the door ;-). Anyway, the API can always be documented (bug 81334). > Martin, can you change your code [..] to [..] table.isSelected([..]) ? The owner draw framework in JFace does not know about the concrete type of the control. We could handle Table as a special case, but most of our owner-draw controls are actually Trees. But Tree doesn't have an isSelected(..) API and using TreeItem[] getSelection() sounds expensive.
Silenio, this is a request for new API. Please advise. Personally I'm not against adding this API, see comment 8 and 9.
The new API is fine. We just need an implementation for the other platforms (GTK and Motif) to be able to close this.
Created attachment 181414 [details] Patch for GTK
(In reply to comment #12) > Created an attachment (id=181414) [details] > Patch for GTK Cool! I can shut down my ubuntu image for a while.
Created attachment 181417 [details] Patch for motif
I have released the code for: Table and Tree on GTK Table and Tree on Motif Table on Windows I was about to release Tree on Windows but my testing showed me that it does not work on all the cases. Change the snippet in the first comment to use a tree, select all items in the tree and just move the mouse over them. Note MeasureItem will report all items as not selected (you will need to add println to the testcase). It seems to me that isItemSelected() is not supposed to be called outside the custom draw phase. Scott, did you have change to test your patch (tree on windows) ? I'll try to fix it.
Created attachment 181444 [details] new patch for Tree on win32 It worked for me. I'll test, verify, and release tomorrow when I get back to the office.
(In reply to comment #15) > It seems to me that isItemSelected() is not supposed to be called outside the > custom draw phase. > > Scott, did you have change to test your patch (tree on windows) ? My test was as you described -- changing the example to use a Tree instead of a Table, but I didn't try the test case you mentioned. I was trying to reuse isItemSelected but it looks like that wasn't a good idea. Silenio and I talked briefly about setting other bits in the detail field, but it looks like we're going to go with just the SELECTED bit now. I'll check in the cocoa change shortly.
Actually, is the point of this change to notify the listener that the item is about to be drawn with the selection or that the item itself is selected? In other words, should we be checking "isSelected && ((style & SWT.HIDE_SELECTION) == 0 || hasFocus" before setting the SELECTED bit?
Tree fixed for win32. Scott, please release cocoa and close the bug. SELECTED in MeasureItem has to mean exactly the same it means in EraseItem and PaintItem. Isn't the current code correct ?
(In reply to comment #19) > Tree fixed for win32. > > Scott, please release cocoa and close the bug. > > SELECTED in MeasureItem has to mean exactly the same it means in EraseItem and > PaintItem. On Cocoa, the code just before we send a PaintItem or EraseItem message is: if (isSelected && ((style & SWT.HIDE_SELECTION) == 0 || hasFocus)) event.detail |= SWT.SELECTED; > Isn't the current code correct ? If I leave that check out and modify the current test case so that the table/tree is created with HIDE_SELECTION the selected item draws with a different text color when the window isn't active and it doesn't look right. So, I think on Cocoa, anyway, we need that extra line. I'll check it all in. It will take me a few minutes to check on windows 7, but it looks like we may need to do something similar there.
(In reply to comment #20) > It will take me a few minutes to check on windows 7, but it looks like we may > need to do something similar there. Yes, I'm seeing the same bug on Windows. With HIDE_SELECTION set the text draws black when the window isn't active.
(In reply to comment #21) > Yes, I'm seeing the same bug on Windows. With HIDE_SELECTION set the text draws > black when the window isn't active. Okay, release all the code you have once you are done testing. I'll see what you did and do the same for GTK and Motif, okay ? To help me test, please add your snippet to this bug. Thanks.
Created attachment 181518 [details] Modified example code Same as the example, except it uses a Tree and sets SWT.HIDE_SELECTION on creation.
Created attachment 181519 [details] Second modified example Table with HIDE_SELECTION
Changes for Cocoa are now checked in.
Awesome, the fix for table used be: event.detail = isSelected (indexOf (item)) ? SWT.SELECTED : 0; Turns out the correct fix is: boolean drawSelected = false; if (OS.IsWindowEnabled (handle)) { LVITEM lvItem = new LVITEM (); lvItem.mask = OS.LVIF_STATE; lvItem.stateMask = OS.LVIS_SELECTED; lvItem.iItem = (int)/*64*/row; int /*long*/ result = OS.SendMessage (handle, OS.LVM_GETITEM, 0, lvItem); boolean selected = (result != 0 && (lvItem.state & OS.LVIS_SELECTED) != 0); if (selected && (column == 0 || (style & SWT.FULL_SELECTION) != 0)) { if (OS.GetFocus () == handle || display.getHighContrast ()) { drawSelected = true; } else { drawSelected = (style & SWT.HIDE_SELECTION) == 0; } } } if (drawSelected) event.detail |= SWT.SELECTED; I'll release Table, then fix Tree, then check GTK and Motif...
Felipe, did you add the security flag on purpose?
It's too easy to check that box. I've done that a couple of times myself.
Thanks Scott, I didn't notice it at all
Is this still on the plan for 3.7?
(In reply to comment #30) > Is this still on the plan for 3.7? All the code has being released (not sure why I left this problem open, I think there was something I wanted to test...) Is the problem fixed on your end ?
> Is the problem fixed on your end ? Yes, looks good. I just removed the workarounds (bug 292198) and tested Tree and Table on Win7, GTK, Cocoa.
(In reply to comment #32) > > Is the problem fixed on your end ? > Yes, looks good. I just removed the workarounds (bug 292198) and tested Tree > and Table on Win7, GTK, Cocoa. thank you Markus, closing this bug as fixed.
