Community
Participate
Working Groups
+++ This bug was initially created as a clone of Bug #277638 +++ DateTime should support DropDown of a Calendar since 35m6 public static void main (String [] args) { Display display = new Display (); Shell shell = new Shell (display); shell.setLayout (new RowLayout ()); DateTime neuerCalendar = new DateTime (shell, SWT.MEDIUM + SWT.DATE + SWT.DROP_DOWN); neuerCalendar.addSelectionListener (new SelectionAdapter () { public void widgetSelected (SelectionEvent e) { System.out.println ("dd changed"); } }); shell.pack (); shell.open (); while (!shell.isDisposed ()) { if (!display.readAndDispatch ()) display.sleep (); } display.dispose (); }
See original bug for GTK work-in-progress.
Arun, this would be a great one to take a look at and get in for M4. Thanks!
I'm currently testing the GTK patch submitted on the original bug. It seems to emulate the behavior of DateTime dropdown on Windows for the most part, except for a couple of minor issues that I have noticed till now. 1) Setting the DateTime's format to SWT.TIME and choosing the DROP_DOWN style setting causes the application to crash with an exception. I added a check to handle this (basically ignore the DROP_DOWN style when the format is not DATE). 2) Hitting Esc key after opening the popup doesn't cause the date to revert to its original value; I'm in the process of fixing this. 3) Hitting Tab key closes the popup calendar and shifts focus to the next control (if there is one). This behavior is different from Windows where the popup calendar remains open and focus doesn't shift out. I'm wondering whether we need to change the behavior to match with the Windows platform. I will upload the complete patch once these issues are sorted out.
Created attachment 183115 [details] Proposed GTK+ patch This patch implements the DROP_DOWN style for DateTime on GTK+ platform. I have fixed the first 2 issues I had pointed out in Comment #3. Please let me know if the behavior described in the 3rd point needs to be fixed or can be left as is. Thanks!
The patch looks mostly OK; as for #3 - what is the behavior when you hit tab in Cocoa?
Some comments: - setMenu() needs a checkWidget(), otherwise it will cause a NullPointerException when called while the widget is disposed. - There are two methods listening for key events (textKeyDown() and gtk_key_press_event()). Is gtk_key_press_event() necessary? It seems textKeyDown() also sends SWT.DefaultSelection. - There ara two methods to layout the widgets (internalLayout() and textResize()). Why? I am not sure internalLayout() is a called at all. - I think we should still keep the "on" prefix on the event methods (textKeyDown() -> onTextKeyDown()). It is easier to identify them as event handlers. - Is OS.GTK_WIDGET_UNSET_FLAGS(down.handle, OS.GTK_CAN_FOCUS); needed in createDropDownButton()? It seems to be needed for the up and down buttons in the non DROP_DOWN case.
(In reply to comment #5) > The patch looks mostly OK; as for #3 - what is the behavior when you hit tab in > Cocoa? Thanks for the review Bogdan. The behavior on Cocoa is the same as on GTK, the popup closes and focus is shifted.
Thanks for the review comments SSQ. I will have a revised patch soon. (In reply to comment #6) > Some comments: > > - setMenu() needs a checkWidget(), otherwise it will cause a > NullPointerException when called while the widget is disposed. Added the call to checkWidget() > > - There are two methods listening for key events (textKeyDown() and > gtk_key_press_event()). Is gtk_key_press_event() necessary? It seems > textKeyDown() also sends SWT.DefaultSelection. It looks like the gtk_key_press_event() handler is not necessary as of now. However, I have just discovered something else that needs to be fixed. Using arrow keys to navigate through the popup calendar is not working. I will investigate and get that fixed and will remove the above handler if it is not required. > > - There ara two methods to layout the widgets (internalLayout() and > textResize()). Why? I am not sure internalLayout() is a called at all. The internalLayout() method is indeed not being used and I have gotten rid of it. I will probably rename textResize() method to onResize() as it is handling all resize events. > > - I think we should still keep the "on" prefix on the event methods > (textKeyDown() -> onTextKeyDown()). It is easier to identify them as event > handlers. I agree, I have made the necessary changes. > > - Is OS.GTK_WIDGET_UNSET_FLAGS(down.handle, OS.GTK_CAN_FOCUS); needed in > createDropDownButton()? It seems to be needed for the up and down buttons in > the non DROP_DOWN case. Yes it is needed only for the non DROP_DOWN case and that is how it is implemented in the current patch, I don't find a call to OS.GTK_WIDGET_UNSET_FLAGS() in createDropDownButton().
Created attachment 183862 [details] Revised patch I have incorporated the suggested changes and also cleaned up event handlers not being used. I have tested the patch and it replicates the behavior of DateTime on Windows, except for issue #3 I discussed in comment #3. Another issue I discovered is that using arrow keys to navigate the popup calendar doesn't seem to work. However, I'm not sure whether this needs to be fixed since the native calendar widget on GTK itself doesn't seem to support this. Thanks!
Hi Arun - we took a look at your patch and have the following suggestions: - Need to add OS.GTK_WIDGET_UNSET_FLAGS(down.handle, OS.GTK_CAN_FOCUS); in create dropDownButton otherwise the drop down button will take focus when you click on it - Setting the foreground/background/font does not work unless the popup shell is recreated somehow. It needs to look like this: public void setForeground(Color color) { super.setForeground(color); fg = color; if (text != null) text.setForeground(color); if (popupCalendar != null) popupCalendar.setForeground(color); } - There are changes made to the date validation code that seems to omit some of the checks that were done before. For example, setDate() doesn't run: if (year < MIN_YEAR || year > MAX_YEAR) return; because it was removed from isValid. We should either add this line back to isValid or in the callers of isValid. Note that you've done the latter in setYear(). - Another problem we've noticed, is that in releaseWidget the popupShell is disposed and nulled out. This is also done in onDispose(). You only need to do this in one place (onDispose). Generally, if you are writing a control using low level APIs you dispose of resources in releaseWidget. In this case, it's a mixture of high level and low level calls, so its fine to dispose resources in a dispose listener. - setMenu needs to call super.setMenu in order for getMenu to return the proper menu. Also, it should set the menu in the buttons (up and down), otherwise there is no pop up over the menus: public void setMenu (Menu menu) { super.setMenu(menu); if (up != null) up.setMenu(menu); if (down != null) down.setMenu(menu); text.setMenu(menu); } Note that checkWidget() is no longer needed as you are calling super() Please take a look at this and get back with another patch. Thanks!
Created attachment 187029 [details] Revised patch Please review this revised patch which includes all the changes suggested above and a few other changes as well. I have added the check which validates the value of year in setDate() since it was missing as rightly pointed out. However, this check is not needed in the other callers of isValid() except for setYear(). This is because only in setYear() and setDate() the value of year is a custom parameter. All the other callers of isValid() use getYear() to fetch the value of year from the system and so it doesn't need to be validated again. I have also added changes to fix the following other problems I noticed with the earlier patch -- the popup calendar doesn't hide when any click happens outside the calendar or when focus is shifted using the keyboard or even when focus is shifted to a different window (via either ALT+TAB or a mouse click).
Thanks Arun. Patch released to HEAD > 20110119
Arun, there were some JUnit failures with the new code: http://download.eclipse.org/eclipse/downloads/drops/N20110120-2000/testresults/html/org.eclipse.swt.tests_linux.gtk.x86_6.0.html You should get out JUnit tests: org.eclipse.swt.tests and run the DateTime GTK suite (or you can just run AllGTKTests and comment everything but the date time tests). I'm going to revert the change for now to let you handle these bugs. Some quick notes: isValid is now calling selectField; this shouldn't happen: the validity of the date has nothing to do with what fields have been populated (the old code never did this) setFormat can lose the checkWidget as it is not API setMenu needs a null check on the text field It's not enough to just check (year < MIN_YEAR || year > MAX_YEAR) in setYear. The previous code would handle things like checking to see if a year contains a Feb 29 (this another of the failed JUnits). It used to call call isValid(year, getMonth(), getDay()) - probably should again. Please take a look at the JUnits failures and attach another patch when everything looks good.
Created attachment 187832 [details] Revised patch Please review the new patch, I have tested to ensure that all the JUnit tests are passed successfully. Thanks!
Created attachment 189307 [details] Updated patch I have incorporated some more changes to the patch for fixing the issue described in the CCombo Bug 335134. I have also reverified that there are no JUnit failures/errors. Please review the updated patch. Thanks!
Created attachment 189309 [details] New patch Please consider this as the updated patch for review and not the previous one. Sorry for the confusion! Thanks!
I looked at the patch and ran the JUnits - they all passed. I compared the behavior vs. Win32/Cocoa and found a problem: if you edit the date field before hitting the drop down, on Win32/Cocoa the calendar will update to show the entered date - on GTK it does not. (For example, change the year to 2012, the drop down calendar will now show today's date in 2012). Another thing I noticed is that both Win32 and Cocoa will show you both the current date and the date you typed in, if they are in the same month. For example, if I change today's date in the field before I hit the drop down (02-22-2011 change to 02-27-2011), the calendar on Win32/Cocoa will show me that today is the 22 and that my selected date is the 27th. Obviously this doesn't matter if I change the month completely. Showing the proper drop down calendar for the entered date is important and it'd be nice if we could add the current date functionality as well. Thanks!
Created attachment 189828 [details] New patch This revised patch addresses both the issues described in comment #17. The dropped down calendar now updates to show the selected date. I have also added code which calls the native GTK calendar API to place a visual marker on the current date when the newly selected date happens to be in the same month as the current month. All JUnit tests passed. Please review the patch and provide your comments/suggestions. Thanks!
Hi, Arun. I have almost finished reviewing this patch, but I need to leave now. I will get back to it on Tuesday. Have a nice weekend!
Hi, Arun. Here are my comments, below. Points 1, 2, and 3 need to be done before we can release this patch, and points 4 and 5 (and some of point 1) are for future work. Please note also that I released the OS changes and recompiled libraries for gtk_calendar_mark_day and gtk_calendar_clear_marks because I knew those would be going in. 1) NPE in DateTime.getSpokenText() because formatSymbols and calendar are not initialized for SWT.CALENDAR. For now, just do something like this in the 'else' in getSpokenText(): /* SWT.DATE or SWT.CALENDAR */ Calendar cal = calendar; if ((style & SWT.CALENDAR) != 0) { formatSymbols = new DateFormatSymbols(); cal = Calendar.getInstance(); getDate(); cal.set(year, month, day); } if ((style & SWT.SHORT) == 0) { result.append(formatSymbols.getWeekdays()[cal.get(Calendar.DAY_OF_WEEK)] + ", "); } result.append(formatSymbols.getMonths()[cal.get(Calendar.MONTH)] + " "); if ((style & SWT.SHORT) == 0) { result.append(cal.get(Calendar.DAY_OF_MONTH) + ", "); } result.append(cal.get(Calendar.YEAR)); } Please open a new bug to rewrite this method using java.text.DateFormat so that it can be locale-sensitive. The current code is awful (I can say that because I wrote it a long time ago <g>) and this needs to be fixed, but not for this patch. 2) If you use only the keyboard to drop down the calendar (Alt+Down arrow) and then use arrow keys to navigate within the calendar, and then type the space key, the date is changed, but the calendar needs to be dismissed. (This should be fixed before we release this patch). 3) Please use the isValidDate() and isValidTime() methods that are in HEAD, and use exactly the code from HEAD for setDate/setDay/setHours/setMinutes/setMonth/setSeconds/setTime/setYear. And delete both isValid() methods. And then re-run the JUnit tests to be sure they still pass. (The original patch contained old code). 4) Please open a new bug to consider deleting the Paint code in popupShellEvent(Event event) that draws a black border around the shell. Here is the code that should probably be deleted, plus where the Paint listener is added: case SWT.Paint: /* Draw black rectangle around popupCalendar */ Rectangle bounds = popupCalendar.getBounds(); Color black = getDisplay().getSystemColor(SWT.COLOR_BLACK); event.gc.setForeground(black); event.gc.drawRectangle(0, 0, bounds.width + 1, bounds.height + 1); break; The new bug should say to first verify that removing the border looks better in other themes... it sure looks better on my machine (Ambiance theme, Ubuntu). 5) Please open a new bug to use gtk_calendar_mark_day for SWT.CALENDAR style also.
Created attachment 190583 [details] Revised patch Thanks for the review Carolyn! Here is the revised patch which addresses points 1, 2 and 3 as discussed. All JUnit tests passed successfully. I have fixed another minor problem I came across which was that the visual marker on the drop down calendar was not getting cleared when the month/year is changed using the mouse. I will raise new bugs for points 4 and 5 (and part of 1) soon. Thanks!
Fixed > 20110307 Thanks, Arun! I released the revised patch. We now have drop-down calendar for SWT GTK! :) I only made 1 trivial change to a comment in gtk_month_changed. I am marking the bug fixed for 3.7 M6. Please open those bugs as soon as you can so that the info is not lost. Thanks!
