#14814 closed enhancement (fixed)
Typed lists in docview
Reported by: | troelsk | Owned by: | |
---|---|---|---|
Priority: | low | Milestone: | |
Component: | GUI-generic | Version: | stable-latest |
Keywords: | docview wxList wxVector | Cc: | |
Blocked By: | Blocking: | ||
Patch: | yes |
Description
While maintaining bw compatibility. No docs yet - needs to know first if this is the right way to implement typed and exported lists nowadays?
Attachments (6)
Change History (24)
Changed 5 years ago by troelsk
comment:1 Changed 5 years ago by vadz
- Priority changed from normal to low
- Status changed from new to infoneeded_new
Why use lists instead of wxVector<>?
In any case, it doesn't seem like a good idea to have GetFoos(vector&) for each and every list GetFoos() in wx API. It would be probably more useful to provide a template converter from wxList to wxVector<> that could be used everywhere.
comment:2 follow-up: ↓ 3 Changed 5 years ago by troelsk
- Status changed from infoneeded_new to new
It would be probably more useful to provide a template converter from wxList to wxVector<> that could be used everywhere.
-1 This will encourage people to keep using the untyped list. wxList is a legacy leftover or?
Why use lists instead of wxVector<>?
+1 wxVector is fine with me, I can make a new patch using it (if it stands a chance of being applied). Esp wxDocList will be very handy to have, really useful in user space, to me at least.
comment:3 in reply to: ↑ 2 Changed 5 years ago by vadz
Replying to troelsk:
It would be probably more useful to provide a template converter from wxList to wxVector<> that could be used everywhere.
-1 This will encourage people to keep using the untyped list. wxList is a legacy leftover or?
Yes, it is, but we're not going to be able to get rid of it because there is too much code using it. And there are dozens if not hundreds of functions returning it in wx, so the thought of adding dozens or hundreds new parallel functions mirroring them just doesn't seem appealing.
Why use lists instead of wxVector<>?
+1 wxVector is fine with me, I can make a new patch using it (if it stands a chance of being applied). Esp wxDocList will be very handy to have, really useful in user space, to me at least.
I do agree that it would be nice to have a type-safe and nicer interface for this and other wxLists but I'm not convinced that adding tons of new functions is a good idea to do it.
Perhaps we should add wxList::AsVector()? Then we'd be able to write
const wxVector<wxWindow*> tlws = wxTopLevelWindows.AsVector(); for ( auto& tlw : tlws ) { ... }
comment:4 Changed 5 years ago by troelsk
wxList...we're not going to be able to get rid of it because there is too much code using it
it doesn't seem like a good idea to have GetFoos(vector&) for each and every list GetFoos() in wx API
I'm only proposing to this to docview, to retire wxList there.
Using docview takes so much casting currently, this is clearly bad.
And this is no better, it is horrible to impose users to write such cruft, in user space,
const wxVector<wxDocument*> docs = docManager->GetDocuments().AsVector<wxDocument>();
This is better, user does not need to learn about wxList and AsVector() at all
wxVector<wxDocument*> docs; docManager->GetDocuments(docs);
Okay with GetFoos() in docview?
I'll make a stab at implementing wxList::AsVector(), to use in docview GetFoos().
comment:5 Changed 5 years ago by troelsk
- Keywords wxList wxVector added
Added a new patch, with wxList::AsVector().
Patch note: I had to remove the wx/utils.h include from vector.h to get list.h to build
(utils.h pulls in GUI stuff, mousestate.h etc)
comment:6 Changed 5 years ago by vadz
Some comments about the patch:
- Calling vectors lists is confusing...
- Why do Get{Views,Documents,Templates}() take a reference instead of just returning the vectors?
- In AsVector() it could be useful to call reserve() before populating the vector. Or even preallocating it with the exact number of elements and then using vec[n] = ... instead of push_back().
- wxCheckCast() is not supposed to be used directly, you should use wxStaticCast(). However both of them only work with classes that use wxRTTI macros and I'm not sure if wxList is always used with objects of these classes.
- I don't understand the point of MakeTemplateArray() and I really don't like not using wxScopedArray for it.
comment:7 follow-up: ↓ 8 Changed 5 years ago by troelsk
Okay. Thanks for feedback.
- Calling vectors lists is confusing...
Ok, changed to wx{View,Doc,Template}Vector
- Why do Get{Views,Documents,Templates}() take a reference instead of just returning the vectors?
Done, these method names are taken, cannot overload by return type only so names now changed to Get{View,Document,Template}Vector()
- In AsVector() it could be useful to call reserve() before populating the vector...
Done
- wxCheckCast() is not supposed to be used directly, you should use wxStaticCast()...wxRTTI macros
Ok, addressing these two issues raised by having two methods, AsVector and AsSafeVector.
- I don't understand the point of MakeTemplateArray() and I really don't like not using wxScopedArray for it.
&templates[0] looks suspect to me, is this really working/correct/safe? Else, MakeTemplateArray() can be removed
BTW, wxScopedArray has no copy ctor
wxScopedArray<wxDocTemplate*> templateArray(new wxDocTemplate*[list.size()]); ... return templateArray; // docview.cpp(1418) : error C2248: 'wxScopedArray<T>::wxScopedArray' : cannot access private member declared in class 'wxScopedArray<T>'
comment:8 in reply to: ↑ 7 Changed 5 years ago by vadz
comment:9 Changed 5 years ago by troelsk
Ok. New patch. Removed MakeTemplateArray() and applied vector in some looping code. If this is to be applied I guess I need to make some documentation?
comment:10 Changed 5 years ago by vadz
Sorry...
I don't like AsSafeVector() thing. If we really need it, we should use some template helper applying either simple static_cast<> or wxStaticCast() depending on whether the latter can be used.
Also, why all the changes to the implementation code? I mean, they're nice and all but hardly necessary. And they actually make the code less efficient. And break its compilation with VC6 because these new functions can't be compiled with it (it doesn't support explicitly choosing the function template specialization to call, see comment near wxCheckCast() definition).
So why not just keep this very simple and add:
- A single AsVector<>.
- The vector-returning accessors (inside #ifndef __VISUALC6__ to not have to deal with that compiler).
- Documentation for them.
And not change anything else?
comment:11 Changed 5 years ago by troelsk
Ok, added new minimal patch, with a little documentation.
(What I really wants from this ticket is actually just the wxDocVector typedef which I'd rather see declared in wx than in some header in my user code)
Thanks
comment:12 Changed 5 years ago by VZ
comment:13 Changed 5 years ago by VZ
comment:14 Changed 5 years ago by VZ
- Resolution set to fixed
- Status changed from new to closed
comment:15 Changed 5 years ago by troelsk
Great, thanks, already put to good use,
http://wxedit.svn.sourceforge.net/viewvc/wxedit/include/wx/ext/wx.h?r1=135&r2=134&pathrev=135
comment:16 Changed 5 years ago by troelsk
[thread.gmane.org/gmane.comp.lib.wxwidgets.devel/145412]
return m_templates.AsVector<wxDocTemplate*>();
Perhaps the compiler stops reporting errors in template code after the
first one.
Nope, if I comment out the first one it does nor report on the 2 others.
Apparantly this compiler needs the full declaration, see new patch "inline.patch"
comment:17 Changed 5 years ago by troelsk
Apparantly this compiler needs the full [wxView class] declaration
Trunk