lmi
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[lmi] Remove a TODO from CensusView implementation


From: Vadim Zeitlin
Subject: [lmi] Remove a TODO from CensusView implementation
Date: Sat, 18 Oct 2014 03:51:48 +0200

 Hello,

 While looking at something completely different (namely, census printing),
I couldn't help noticing these lines in the same census_view.cpp file:

        // TODO ?? Can't this macro be dispensed with?
        #define ID_LISTWINDOW 12345

And the answer is, of course, that we can avoid using it and the patch
below does exactly this. Notice that the patch is probably too paranoid and
the check added to CensusView::UponRightClick() could be just omitted, as
we are only going to receive events from the only wxDataViewCtrl we're
using right now, but, as written, it exactly preserves the semantics of the
code.

 A better solution would be to not use event table for connecting this
event handler at all and instead call Bind() to do it after creating
list_window_. However Bind() is not currently used in LMI code and while I
do think that it would be a good idea to start using it, I didn't want to
change too many things just to deal with this simple case.

 As always, please let me know if you have any questions about this patch,
VZ

>From 9de5e590a2e3f241c3b22b9420e617868f9fa5c0 Mon Sep 17 00:00:00 2001
From: Vadim Zeitlin <address@hidden>
Date: Sat, 18 Oct 2014 03:43:59 +0200
Subject: [PATCH] Get rid of hardcoded constant ID for wxDataViewCtrl in
 CensusView.

There is no real reason to use a fixed ID for this control, we don't really
care about its value and we have just a single wxDataViewCtrl in this window,
so we don't need to use its ID at all, in principle.

But, just for extra safety, do check that we only get the events from the
expected control and ignore them otherwise.

---
 census_view.cpp | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/census_view.cpp b/census_view.cpp
index d4632fc..8f52c90 100644
--- a/census_view.cpp
+++ b/census_view.cpp
@@ -65,9 +65,6 @@
 #include <iterator>
 #include <sstream>
 
-// TODO ?? Can't this macro be dispensed with?
-#define ID_LISTWINDOW 12345
-
 namespace
 {
 // TODO ?? Add description and unit tests; consider relocating,
@@ -803,7 +800,7 @@ wxString CensusViewDataViewModel::GetColumnType(unsigned 
int col) const
 IMPLEMENT_DYNAMIC_CLASS(CensusView, ViewEx)
 
 BEGIN_EVENT_TABLE(CensusView, ViewEx)
-    EVT_DATAVIEW_ITEM_CONTEXT_MENU(ID_LISTWINDOW, CensusView::UponRightClick)
+    EVT_DATAVIEW_ITEM_CONTEXT_MENU(wxID_ANY ,CensusView::UponRightClick)
     EVT_MENU(XRCID("edit_cell"             ),CensusView::UponEditCell )
     EVT_MENU(XRCID("edit_class"            ),CensusView::UponEditClass)
     EVT_MENU(XRCID("edit_case"             ),CensusView::UponEditCase )
@@ -957,7 +954,7 @@ wxWindow* CensusView::CreateChildWindow()
 {
     list_window_ = new(wx) wxDataViewCtrl
         (GetFrame()
-        ,ID_LISTWINDOW
+        ,wxID_ANY
         ,wxDefaultPosition
         ,wxDefaultSize
         ,wxDV_ROW_LINES | wxDV_MULTIPLE
@@ -1364,8 +1361,17 @@ void CensusView::UponColumnWidthFixed(wxCommandEvent&)
         }
 }
 
-void CensusView::UponRightClick(wxDataViewEvent&)
+void CensusView::UponRightClick(wxDataViewEvent& e)
 {
+    if(e.GetEventObject() != list_window_)
+        {
+        // We're only interested in these events from our own internal window.
+        // Normally we shouldn't be getting any other events, but if we do,
+        // just ignore them.
+        e.Skip();
+        return;
+        }
+
     wxMenu* census_menu = wxXmlResource::Get()->LoadMenu("census_menu_ref");
     LMI_ASSERT(census_menu);
     list_window_->PopupMenu(census_menu);
-- 
2.1.0.rc0

reply via email to

[Prev in Thread] Current Thread [Next in Thread]