Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#12042 closed enhancement (fixed)

wxQt: wxCalendarCtrl implementation

Reported by: kosenko Owned by: Peter_Most
Priority: normal Milestone: future
Component: wxQt Version:
Keywords: wxCalendarCtrl wxQt Cc:
Blocked By: Blocking:
Patch: yes

Description

I would like to introduce calendar control implementation for wxQt port.

  1. Apply wxqt_calctrl.patch to wxQt branch
  2. Regenerate bakefiles, (run autoconf,) configure and make wxQt
  3. Apply wxqt_sample_minimal_calctrl.patch to the minimal sample
  4. Calendar is a part of wxAdv library but minimal sample do not uses this library. So you need to manually add linking with this library with parameter similar to -lwx_qtu_adv-2.9 in Makefile of this sample
  5. Make and run minimal sample
  6. Enjoy!

Attachments (4)

wxqt_calctrl.patch download (20.1 KB) - added by kosenko 8 years ago.
source code
wxqt_sample_minimal_calctrl.patch download (5.6 KB) - added by kosenko 8 years ago.
code for testing
wxqt_calctrl_on_gnome.png download (104.2 KB) - added by kosenko 8 years ago.
wxGTK and wxQt calendars on GNOME
wxqt_calctrl_on_kde.png download (108.2 KB) - added by kosenko 8 years ago.
wxGTK and wxQt calendars on KDE

Download all attachments as: .zip

Change History (16)

Changed 8 years ago by kosenko

source code

Changed 8 years ago by kosenko

code for testing

Changed 8 years ago by kosenko

wxGTK and wxQt calendars on GNOME

Changed 8 years ago by kosenko

wxGTK and wxQt calendars on KDE

comment:1 follow-up: Changed 8 years ago by Peter Most

  • Owner set to Peter Most
  • Status changed from new to accepted

Thanks for the patch, but could you please put wxQtCalendarCtrl in it's own file (calctrl_qt.*)? This way we don't need to add a special build rule.
You wrote in void wxCalendarCtrl::RefreshHolidays(): 'Bug in Qt: returned background color is incorrect'. Do you have a link for this bug which we could embed so we can check it from time to time?

comment:2 in reply to: ↑ 1 ; follow-up: Changed 8 years ago by kosenko

Replying to Peter Most:

Thanks for the patch, but could you please put wxQtCalendarCtrl in it's own file (calctrl_qt.*)? This way we don't need to add a special build rule.

I do not have used separated cpp modules for internal classes intentionally because of:

  1. wxQtCalendarCtrl takes only 29 lines of code and I think it will not be much bigger in the future. So it looks like very small class to have exclusive two files.
  2. calctrl.cpp will not be supported by moc preprocessor as it is supported now.
  3. I suppose that we will have related to a specific control classes in the future, so each of this classes should have its own two additional files?
  4. calctrl.cpp requires -DwxUSE_CALENDARCTRL flag to moc to create moc file, so common build rule will not work without changes.
  5. Adding new build rule will take a few minutes and this rules in quite simple.
  6. Other ports like wxMSW do not exposes internal classes, just suppose what will be if each internal class in wxMSW port have own two files.

Having each class in its own file also have some benefits but in this case I think it would be better do not expose external files. I hope that you will appreciate this approach and use it for other modules if this is acceptable.

You wrote in void wxCalendarCtrl::RefreshHolidays(): 'Bug in Qt: returned background color is incorrect'. Do you have a link for this bug which we could embed so we can check it from time to time?

I do not searched this bug in Qt bug tracer. I have created Qt control but it returns background black at least for the first time, so I suppose it is a bug in Qt because actually QCalendarWidget has no black backgrounds on my screen. I have tested on Qt 4.5.0. Changing background colour is not main stream usage of calendar anyhow, so I think we may do not much care about this now.

I am going to test this control more on calendar sample after you have finish working on menus. Also it would be nice to have sizers support or absolute positioning at least.

comment:3 Changed 8 years ago by kosenko

  • Component changed from GUI-all to wxQt

comment:4 in reply to: ↑ 2 ; follow-up: Changed 8 years ago by Peter Most

  • Status changed from accepted to infoneeded
  1. calctrl.cpp will not be supported by moc preprocessor as it is supported now.

I don't understand what you mean here.

  1. I suppose that we will have related to a specific control classes in the future, so each of this classes should have its own two additional files?

Yes, for each Qt widget, we need to use, we create a separate class like wxQtCalendarCtrl. Because it must be processed by moc we should put it in a separate file, so that the current build rule will process it. Maybe if you look at the action_qt.* files and the corresponding section in files.bkl then you understand better what I mean.

  1. calctrl.cpp requires -DwxUSE_CALENDARCTRL flag to moc to create moc file, so common build rule will not work without changes.

Couldn't we solve this if we included the 'setup.h' file?

  1. Adding new build rule will take a few minutes and this rules in quite simple.

But with your approach, each derived qt class would need it's own build rule!

  1. Other ports like wxMSW do not exposes internal classes, just suppose what will be if each internal class in wxMSW port have own two files.

They don't need to support moc ;-)

If you know of a better way of how to include the moc pre-processor, but without a separate build rule for every class, then I gladly discuss it with you. But I think the mailing list is more suited for this discussion.

comment:5 in reply to: ↑ 4 ; follow-up: Changed 8 years ago by kosenko

  • Status changed from infoneeded to accepted

without a separate build rule for every class

Why you are thinking that separated build rules is bad? With adding new file developer should add this file not only to files.bkl, but also to qtrules.mk for moc support?

Having more files in three times in src/qt folder is better?

comment:6 in reply to: ↑ 5 ; follow-up: Changed 8 years ago by Peter Most

  • Status changed from accepted to infoneeded

Why you are thinking that separated build rules is bad? With adding new file developer should add this file not only to files.bkl, but also to qtrules.mk for moc support?

Because a developer should only have to add files to one file (files.bkl) and not to two. Besides, what happens when bakefile gets extended to support moc on other platforms? Then the qtrules.mk file will probably not be needed anymore.

Having more files in three times in src/qt folder is better?

What's so terrible about having a couple more files in src/qt? When you think about hiding the wxQt* classes, then you have to consider that embedding these classes in the cpp file only hides it for the compiler, the linker still sees the names and can therefore still lead to name clashes in the user code.

comment:7 in reply to: ↑ 6 Changed 8 years ago by kosenko

  • Status changed from infoneeded to accepted

Replying to Peter Most:

Besides, what happens when bakefile gets extended to support moc on other platforms? Then the qtrules.mk file will probably not be needed anymore.

  1. On other platforms we can have qtrules.bat file in addition to qtrules.mk files. Additional one file is very terrible in comparison hundred new files in src/qt?
  2. How we can discuss porting to other platforms while most of controls is not implemented at all. Do you expect ready solution how it will work on other platform?
  3. Radically improving of qtrules.mk file is not subject of this ticket. This may be done in the future.

When you think about hiding the wxQt* classes, then you have to consider that embedding these classes in the cpp file only hides it for the compiler, the linker still sees the names and can therefore still lead to name clashes in the user code.

It is not a problem:

  1. wxMSW has many internal classes, but this classes is started from wx prefix. User's classes should not have wx prefix to avoid conflicts with public and internal wx classes.
  2. We can use anonymous namespaces to hide internal classes.
  3. New approach do not changes this, this classes will be visible for linker in both cases.

Peter, do you interested in calendar implementation or not?

comment:8 follow-up: Changed 8 years ago by Peter Most

  • Resolution set to fixed
  • Status changed from accepted to closed

I've added the implementation (r64311) and I think I found a good compromise which keeps the number of files down and we don't need to have custom build rules. Please have a look a http://wiki.wxwidgets.org/WxQt were I try to explain it.

comment:9 in reply to: ↑ 8 ; follow-up: Changed 8 years ago by kosenko

Replying to Peter Most:

I've added the implementation (r64311) and I think I found a good compromise which keeps the number of files down and we don't need to have custom build rules.

I like this solution. Also it will be possible to derive both wxWidgets and corresponding Qt classes in user code in the future.

You have added wxMISSING_IMPLEMENTATION to background formats code and it will be called each time when calendar control is used. This feature is not critical and it is not implemented at all on some other ports. Also not all calendar styles can be implemented in each port but there is no similar marks for it in the code, but it is mentioned in documentation.

comment:10 in reply to: ↑ 9 Changed 8 years ago by Peter Most

Sorry for the late answer.

You have added wxMISSING_IMPLEMENTATION to background formats code and it will be called each time when calendar control is used. This feature is not critical and it is not implemented at all on some other ports. Also not all calendar styles can be implemented in each port but there is no similar marks for it in the code, but it is mentioned in documentation.

I'm not sure which code you mean, but I probably added it because your comments suggested that there is still something missing. If you think it should be removed, then I gladly accept another patch ;-)
But you should bear in mind, that the current implementation of wxMissingImplementation() could be changed so as not to print these messages.

comment:11 Changed 8 years ago by kosenko

I told about following lines of code:

    wxMISSING_IMPLEMENTATION( "Get holiday background color" );
    wxMISSING_IMPLEMENTATION( "Set holiday background color" );

comment:12 Changed 8 years ago by wxsite

  • Owner changed from Peter Most to Peter_Most
Note: See TracTickets for help on using tickets.