Opened 17 months ago

Closed 17 months ago

Last modified 17 months ago

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

typed-lists.patch download (6.6 KB) - added by troelsk 17 months ago.
Trunk
typed-lists2.patch download (7.7 KB) - added by troelsk 17 months ago.
wxList.AsVector
typed-lists3.patch download (8.0 KB) - added by troelsk 17 months ago.
AsSafeVector
typed-lists4.patch download (10.4 KB) - added by troelsk 17 months ago.
Removed MakeTemplateArray() and applied vector in some looping code
typed-lists5.patch download (7.1 KB) - added by troelsk 17 months ago.
Minimal version
inline.patch download (1.5 KB) - added by troelsk 17 months ago.
inline

Download all attachments as: .zip

Change History (24)

Changed 17 months ago by troelsk

Trunk

comment:1 Changed 17 months 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: Changed 17 months 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 17 months 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 17 months 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().

Changed 17 months ago by troelsk

wxList.AsVector

comment:5 Changed 17 months 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 17 months ago by vadz

Some comments about the patch:

  1. Calling vectors lists is confusing...
  2. Why do Get{Views,Documents,Templates}() take a reference instead of just returning the vectors?
  3. 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().
  4. 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.
  5. I don't understand the point of MakeTemplateArray() and I really don't like not using wxScopedArray for it.

Changed 17 months ago by troelsk

AsSafeVector

comment:7 follow-up: Changed 17 months ago by troelsk

Okay. Thanks for feedback.

  1. Calling vectors lists is confusing...

Ok, changed to wx{View,Doc,Template}Vector

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

  1. In AsVector() it could be useful to call reserve() before populating the vector...

Done

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

  1. 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 17 months ago by vadz

Replying to troelsk:

&templates[0] looks suspect to me, is this really working/correct/safe?

If templates is a vector, yes. Vector is guaranteed to store its items in contiguous memory.

BTW, wxScopedArray has no copy ctor

There is no good (i.e. not std::auto_ptr<>-like) solution to this in C++98.

Changed 17 months ago by troelsk

Removed MakeTemplateArray() and applied vector in some looping code

comment:9 Changed 17 months 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 17 months 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:

  1. A single AsVector<>.
  2. The vector-returning accessors (inside #ifndef __VISUALC6__ to not have to deal with that compiler).
  3. Documentation for them.

And not change anything else?

comment:11 Changed 17 months 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

Changed 17 months ago by troelsk

Minimal version

comment:12 Changed 17 months ago by VZ

(In [73002]) Don't include wx/utils.h from wx/vector.h.

This will allow to include wx/vector.h from wx/list.h which is itself included
from wx/utils.h by breaking this circular dependency.

Don't use wxMin(), defined in wx/utils.h, in order to do this.

See #14814.

comment:13 Changed 17 months ago by VZ

(In [73003]) Add wxList::AsVector<>() helper.

This can be useful in legacy code using wxList to progressively replace it
with wxVector.

See #14814.

comment:14 Changed 17 months ago by VZ

  • Resolution set to fixed
  • Status changed from new to closed

(In [73004]) Add wxDocManager::Get{Views,Documents,Templates}Vector().

Add accessors returning more convenient wxVectors to supplement the existing
ones giving access to internally used wxLists.

Closes #14814.

Changed 17 months ago by troelsk

inline

comment:16 Changed 17 months 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 17 months ago by troelsk

Apparantly this compiler needs the full [wxView class] declaration

comment:18 Changed 17 months ago by VZ

(In [73048]) Compilation fix for wxDocManager after r73004.

Define GetXXXVector() methods after all the classes are fully declared to
ensure that static_cast<> inside wxList::AsVector() they use compiles with the
OpenVMS compiler.

See #14814.

Note: See TracTickets for help on using tickets.