From a first sight:
- The dm_~ functions are HIDDEN, i.e. they won't be visible in other source files.
- The year numbers in the new files header aren't correct.
- The return of qt_draw() looks odd (?)
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
First of all thanks for your feedback. I modified the patch according to your suggestions.
Initially, I have used HIDDEN functions because I followed the X dm source and also because the functions are accessible from the dm_qt struct.
Sorry about the years.
It was a little bit odd the return statement (I used that form because I couldn’t find a way to mark the callback_function parameter as unused). I still have this problem even though I found out another way to deal with it. Also, I have added a comment there to be more clear.
The HIDDEN was OK, the prototypes in dm-qt.h are not. Functions only visible (i.e. static/HIDDEN) in a single source file aren't exportet. I.e. to declare them for use in other files is useless.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Next iteration:
include/dm.h:
- Line 54 "#define DM_QT (struct dm *)NULL" has to be removed. DM_QT should be a compiler flag define set by CMake. I.e. based on a BRLCAD_ENABLE_QT CMake variable (there we need an additional test for an existing Qt installation later).
- Why isn't DM_TYPE_QT = 10?
- Line 129: copy-n-paste error
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Solved all issues.
- Added BRLCAD_ENABLE_QT to cmake.
- It was 11 beacuse when I initally started working at the patch I had also txtDM which was 10
- There wasn't a copy-paste error. I misunderstood what NEED_GUI from attach.c does and that's why it was DM_NULL
- I have also made some changes in attach.c
Since this substantially has the same problems as your debug/txt dm patch and you now have commit access, you can directly fix the issues with this patch as you commit changes. Remember to commit early, commit often, and don't break anything. ;)
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
From a first sight:
- The dm_~ functions are HIDDEN, i.e. they won't be visible in other source files.
- The year numbers in the new files header aren't correct.
- The return of qt_draw() looks odd (?)
Hi,
First of all thanks for your feedback. I modified the patch according to your suggestions.
Initially, I have used HIDDEN functions because I followed the X dm source and also because the functions are accessible from the dm_qt struct.
Sorry about the years.
It was a little bit odd the return statement (I used that form because I couldn’t find a way to mark the callback_function parameter as unused). I still have this problem even though I found out another way to deal with it. Also, I have added a comment there to be more clear.
Thanks again.
The HIDDEN was OK, the prototypes in dm-qt.h are not. Functions only visible (i.e. static/HIDDEN) in a single source file aren't exportet. I.e. to declare them for use in other files is useless.
You are right. The dm-qt.h is useless for HIDDEN functions. I have updated the patch so that it does not contain the dm-qt.h.
Next iteration:
include/dm.h:
- Line 54 "#define DM_QT (struct dm *)NULL" has to be removed. DM_QT should be a compiler flag define set by CMake. I.e. based on a BRLCAD_ENABLE_QT CMake variable (there we need an additional test for an existing Qt installation later).
- Why isn't DM_TYPE_QT = 10?
- Line 129: copy-n-paste error
Solved all issues.
- Added BRLCAD_ENABLE_QT to cmake.
- It was 11 beacuse when I initally started working at the patch I had also txtDM which was 10
- There wasn't a copy-paste error. I misunderstood what NEED_GUI from attach.c does and that's why it was DM_NULL
- I have also made some changes in attach.c
Last edit: Vlad Bogolin 2013-06-04
Only one thing: I personally would make two empty lines before and after qt_open_dm() in dm-qt.c.
I can't see any more issues, i.e. the patch is OK.
A remark on the UNUSED() in qt_draw() - the following should work (but you don't need it!):
typedef struct bn_vlist (dm_draw_callback)(void *);
HIDDEN int
qt_draw(struct dm UNUSED(dmp), dm_draw_callback UNUSED(callback_function), genptr_t UNUSED(data))
Since this substantially has the same problems as your debug/txt dm patch and you now have commit access, you can directly fix the issues with this patch as you commit changes. Remember to commit early, commit often, and don't break anything. ;)