wxNamingViewer: build with wxWidgets 3.2 #2169
Conversation
|
Thanks for working on this. Would it be feasible to update one of the GitHub Actions builds to enable wxWidgets? |
saper
force-pushed
the
wxwidgets-3-2-support
branch
from
a13fcf5 to
d690c4d
Compare
wxNamingTree.cpp:94:45: error: non-const lvalue reference to type 'wxTreeItemIdValue'
(aka 'void *') cannot bind to a value of unrelated type 'long'
wxTreeItemId child = GetFirstChild( item, cookie);
^~~~~~
LLVM 16 fails to compile this, GetParent() seem to refer to wxWindow *GetParent() which has no args.
Going back to the source code from 2003, it seems that it has always should have been wxCommandEvent and not wxMouseEvent.
To get number of items in the container, GetCount() should be used. Hard to tell when Number() has been removed.
Sure, tried this 74ba32c but it looks like the build does not even descend into the |
|
It's running the makefile but not building anything yet. |
saper
force-pushed
the
wxwidgets-3-2-support
branch
from
74ba32c to
5ec78ec
Compare
WalkthroughThe changes introduce a new wxWidgets feature configuration to the GitHub Actions workflow and update several wxWidgets-based dialog implementations. The modifications include conditional compilation directives to adjust window border styles (switching between Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant D as WxViewIORDialog
U->>D: Click "Apply" button
D->>D: Invoke OnApply(event)
alt Ancient wxWidgets
D->>D: Call wxDialog::OnApply(event)
else Modern wxWidgets
D->>D: Call wxDialog::EndModal(wxID_APPLY)
end
D->>D: Decode IOR & handle errors
D-->>U: Close dialog / Return control
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
TAO/utils/wxNamingViewer/wxViewIORDialog.cpp (1)
164-164: Fix incorrect assertion variable.The assertion is checking
typeIDTextbut should be checkingprofilessince that's the variable that was just assigned.- assert( typeIDText); + assert( profiles);
🧹 Nitpick comments (5)
TAO/utils/wxNamingViewer/wxViewIORDialog.cpp (1)
222-234: LGTM! Consider adding documentation for the macro.The macro and event handling changes are well-implemented, providing clean compatibility across wxWidgets versions.
Add a brief comment explaining the purpose of the
ONLY_ANCIENT_WX_USESmacro:+// Macro to handle event parameters differently for older wxWidgets versions +// where event parameters were required to be used #if defined(ANCIENT_WX_WINDOWS) #define ONLY_ANCIENT_WX_USES(x) (x) #else #define ONLY_ANCIENT_WX_USES(x) WXUNUSED(x) #endifTAO/utils/wxNamingViewer/wxNamingTree.cpp (2)
159-161: Consider implementing the TODO items for better UX.The TODO comments indicate missing features that would improve user experience:
- Different icons for contexts
- Conditional setting of children flag
Would you like me to help implement these features or create issues to track them?
396-421: Consider refactoring duplicated unbind logic.The unbind logic is duplicated between
onContextPopupUnbindandonObjectPopupUnbind. Consider extracting the common code into a shared helper method.+private: + void unbindItem(const wxTreeItemId& item, const wxString& confirmMessage) { + if (wxMessageBox( + confirmMessage, + "Confirm", + wxYES_NO | wxICON_QUESTION) != wxYES) { + return; + } + wxTreeItemId parentItem = GetItemParent(item); + if (parentItem == 0) { + return; + } + WxNamingObject* object = getTreeObject(item); + WxNamingObject* parent = getTreeObject(parentItem); + CosNaming::NamingContext_var context = parent->NamingContext(); + try { + context->unbind(object->Name()); + clearChildren(item); + Delete(item); + } catch(CORBA::Exception& ex) { + wxMessageBox(ex._rep_id(), "CORBA::Exception"); + } + } void WxNamingTree::onContextPopupUnbind(wxCommandEvent&) { - if (wxMessageBox( - "Are you sure you want to unbind this context?", - "Confirm", - wxYES_NO | wxICON_QUESTION) != wxYES) { - return; - } - wxTreeItemId item = GetSelection(); - wxTreeItemId parentItem = GetItemParent(item); - if (parentItem == 0) { - return; - } - WxNamingObject* object = getTreeObject(item); - WxNamingObject* parent = getTreeObject(parentItem); - CosNaming::NamingContext_var context = parent->NamingContext(); - try { - context->unbind(object->Name()); - clearChildren(item); - Delete(item); - } catch(CORBA::Exception& ex) { - wxMessageBox(ex._rep_id(), "CORBA::Exception"); - } + unbindItem(GetSelection(), "Are you sure you want to unbind this context?"); } void WxNamingTree::onObjectPopupUnbind(wxCommandEvent&) { - if (wxMessageBox( - "Are you sure you want to unbind this object?", - "Confirm", - wxYES_NO | wxICON_QUESTION) != wxYES) { - return; - } - wxTreeItemId item = GetSelection(); - wxTreeItemId parentItem = GetItemParent(item); - if (parentItem == 0) { - return; - } - WxNamingObject* object = getTreeObject(item); - WxNamingObject* parent = getTreeObject(parentItem); - CosNaming::NamingContext_var context = parent->NamingContext(); - try { - context->unbind(object->Name()); - clearChildren(item); - Delete(item); - } catch(CORBA::Exception& ex) { - wxMessageBox(ex._rep_id(), "CORBA::Exception"); - } + unbindItem(GetSelection(), "Are you sure you want to unbind this object?"); }Also applies to: 344-367
TAO/utils/wxNamingViewer/wxBindDialog.cpp (1)
85-91: LGTM! Border style change is consistent.The conditional compilation for window styles matches other dialog implementations. Consider extracting the common window style flags into a macro or constant to reduce code duplication across dialog classes.
// Add to a common header file: #if wxABI_VERSION < 20800 #define WX_DIALOG_BORDER_STYLE (wxTHICK_FRAME) #else #define WX_DIALOG_BORDER_STYLE (wxRESIZE_BORDER) #endif // Use in dialog constructors: wxRAISED_BORDER | wxCAPTION | WX_DIALOG_BORDER_STYLE | wxSYSTEM_MENUTAO/utils/wxNamingViewer/wxSelectNSDialog.cpp (1)
146-150: Consider removing commented-out code.The old code using
Number()is commented out but still present. Since the change toGetCount()is confirmed working, the old code can be safely removed.-#if 0 - int count = servers->Number(); -#else int count = servers->GetCount(); -#endif
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/linux.yml(1 hunks)TAO/utils/wxNamingViewer/wxAddNameServerDlg.cpp(1 hunks)TAO/utils/wxNamingViewer/wxBindDialog.cpp(1 hunks)TAO/utils/wxNamingViewer/wxBindNewContext.cpp(1 hunks)TAO/utils/wxNamingViewer/wxNamingTree.cpp(4 hunks)TAO/utils/wxNamingViewer/wxNamingViewer.cpp(0 hunks)TAO/utils/wxNamingViewer/wxNamingViewer.wxr(5 hunks)TAO/utils/wxNamingViewer/wxSelectNSDialog.cpp(4 hunks)TAO/utils/wxNamingViewer/wxSelectNSDialog.h(1 hunks)TAO/utils/wxNamingViewer/wxViewIORDialog.cpp(2 hunks)
💤 Files with no reviewable changes (1)
- TAO/utils/wxNamingViewer/wxNamingViewer.cpp
🔇 Additional comments (18)
TAO/utils/wxNamingViewer/wxViewIORDialog.cpp (2)
137-143: LGTM! Good approach for maintaining backward compatibility.The conditional compilation directive appropriately handles different wxWidgets versions by switching between
wxTHICK_FRAMEandwxRESIZE_BORDERbased on the ABI version.
189-194: LGTM! Good defensive programming.The early return for nil objects prevents potential issues and properly handles the UI state.
TAO/utils/wxNamingViewer/wxNamingTree.cpp (4)
93-93: LGTM: Correct type for wxWidgets tree item cookie.The change from
longtowxTreeItemIdValuealigns with the wxWidgets API requirements for tree item cookies.
314-314: LGTM: Updated to use preferred wxWidgets API.The change from
GetParenttoGetItemParentfollows wxWidgets best practices and ensures compatibility with version 3.2.
353-353: LGTM: Consistent use of preferred wxWidgets API.The change maintains consistency with the updated wxWidgets API usage throughout the codebase.
407-407: LGTM: Consistent use of preferred wxWidgets API.The change completes the consistent update to the preferred wxWidgets API across all methods.
TAO/utils/wxNamingViewer/wxSelectNSDialog.h (1)
22-22: Verify event handling compatibility with wxListBox.The change from
wxMouseEventtowxCommandEventforonLeftDClickaligns with wxWidgets 3.2's event handling, as list controls now emit command events for double-clicks. However, let's verify this is properly connected in the implementation.✅ Verification successful
Event handling in wxSelectNSDialog for list controls is correctly updated.
- The event table binds
EVT_LISTBOX_DCLICKonIDC_SERVERStoonLeftDClick.- The implementation of
onLeftDClickaccepts awxCommandEvent, which is consistent with wxWidgets 3.2 practices.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for event table entries and event handler implementations echo "Searching for event table entries:" rg -A 2 "BEGIN_EVENT_TABLE.*WxSelectNSDialog" echo "Searching for event handler implementation:" rg "void.*WxSelectNSDialog::onLeftDClick" # Search for list control event bindings echo "Searching for list control event bindings:" rg "EVT_LISTBOX_DCLICK|EVT_LIST_ITEM_ACTIVATED"Length of output: 920
TAO/utils/wxNamingViewer/wxBindNewContext.cpp (2)
98-105: LGTM! Border style change is correctly implemented.The conditional compilation for window styles properly handles compatibility between wxWidgets versions:
- Uses
wxTHICK_FRAMEfor versions < 2.8.0- Uses
wxRESIZE_BORDERfor newer versions
98-105: LGTM! Window style update is correctly implemented.The conditional compilation directive for window styles is well-structured and maintains backward compatibility while supporting wxWidgets 3.2.
TAO/utils/wxNamingViewer/wxAddNameServerDlg.cpp (2)
101-107: LGTM! Border style change matches other dialog implementations.The conditional compilation for window styles is consistent with other dialog classes in the codebase.
101-107: LGTM! Window style update matches the pattern.The window style changes are consistent with other dialog implementations in the codebase.
TAO/utils/wxNamingViewer/wxNamingViewer.wxr (1)
1-2: LGTM! Consistent dialog style updates.The dialog styles have been consistently updated to use
wxRESIZE_BORDERacross all dialogs, which aligns with modern wxWidgets practices.Also applies to: 29-30, 51-52, 73-74, 95-96
TAO/utils/wxNamingViewer/wxSelectNSDialog.cpp (3)
96-102: LGTM! Proper version-based dialog style handling.The conditional compilation ensures compatibility with different wxWidgets versions by using the appropriate frame style.
165-165: LGTM! Proper string conversion.The use of
AsChar()ensures proper conversion of wxString to char* for the configuration API.Also applies to: 168-168
193-193: LGTM! Correct event type for double-click handling.Changed from
wxMouseEventtowxCommandEventwhich is the correct event type for list box double-click events..github/workflows/linux.yml (1)
161-168: LGTM! Proper CI configuration for wxWidgets support.The new feature matrix entry correctly:
- Uses a compatible compiler (gcc-10)
- Installs required wxWidgets packages
- Sets the necessary build flags (
wxWindows=1)TAO/utils/wxNamingViewer/wxBindDialog.cpp (2)
85-91: LGTM! Window style update follows the established pattern.The window style changes maintain consistency with other dialog implementations while preserving backward compatibility.
85-91: Consider documenting the version-based window style changes.The consistent use of preprocessor directives for handling wxWidgets version differences is well-implemented across all dialog classes. As mentioned in the PR objectives, if there's no need to support older frameworks, these conditional directives could be removed in the future.
Let's verify if there are any other files in the codebase that might need similar updates:
Also applies to: 101-107, 98-105
✅ Verification successful
Architectural Note on Version-Based Window Style Logic
The codebase consistently employs preprocessor directives in multiple dialog classes to handle wxWidgets version differences. Our verification confirms that the following files use this approach:
- TAO/utils/wxNamingViewer/wxBindDialog.cpp (lines 85–91)
- TAO/utils/wxNamingViewer/wxAddNameServerDlg.cpp
- TAO/utils/wxNamingViewer/wxSelectNSDialog.cpp
- TAO/utils/wxNamingViewer/wxViewIORDialog.cpp
- TAO/utils/wxNamingViewer/wxBindNewContext.cpp
This consistent design ensures maintainability and smooth backward compatibility. However, documenting the rationale behind these version-specific style flags (e.g., when using
wxTHICK_FRAMEversuswxRESIZE_BORDER) would further assist future developers—especially if support for older wxWidgets versions is eventually dropped.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other dialog implementations that might need similar updates rg -l "wxTHICK_FRAME" --type cppLength of output: 259
The patches attached are needed to build and
run wxNamingViewer with wxWidgets 3.2
For most existing code I could not identify which version
of wxWindows/wxWidgets it could have worked last time,
therefore a generic #ifdef has been used.
If no support for old frameworks is needed,
those could be removed.
Summary by CodeRabbit