Menu

#185 Qt display manager

Incomplete
closed-rejected
None
5
2013-06-21
2013-06-01
No

A first step in implementing the qt display manager. Now it only prints logs similar to txt dm, but has some improvements.

1 Attachments

Discussion

  • Daniel Roßberg

    Daniel Roßberg - 2013-06-03

    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 (?)

     
    • Vlad Bogolin

      Vlad Bogolin - 2013-06-03

      Hi,

      First of all thanks for your feedback. I modified the patch according to your suggestions.

      1. 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.

      2. Sorry about the years.

      3. 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.

       
  • Daniel Roßberg

    Daniel Roßberg - 2013-06-04

    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.

     
    • Vlad Bogolin

      Vlad Bogolin - 2013-06-04

      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.

       
  • Daniel Roßberg

    Daniel Roßberg - 2013-06-04

    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

     
    • Vlad Bogolin

      Vlad Bogolin - 2013-06-04

      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
  • Daniel Roßberg

    Daniel Roßberg - 2013-06-05

    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))

     
  • Sean Morrison

    Sean Morrison - 2013-06-21

    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. ;)

     
  • Sean Morrison

    Sean Morrison - 2013-06-21
    • status: open --> closed-rejected
    • assigned_to: Vlad Bogolin
     

Log in to post a comment.