Opened 6 years ago

Last modified 4 years ago

#13787 confirmed enhancement

wxAuiNotebook tab alignment

Reported by: R.U.10 Owned by:
Priority: normal Milestone: 3.2.0
Component: wxAui Version: stable-latest
Keywords: wxAuiNotebook tab alignment Cc: debian@…
Blocked By: #14756, #14756, #14756, #14756, #14756, #14756, #14756, #14756, #14756, #14756, #14756, #14756, #14756, #14756, #14756, #14756, #14756, #14756, #14756, #14756, #14756, #14756, #14756, #14756, #14756, #14756, #14756, #14756, #14756, #14756, #14756, #14756, #14756, #14756, #14756, #14756, #14756, #14756, #14756, #14756, #14756, #14756, #14756, #14756, #14756, #14756, #14756, #14756, #14756, #14756, #14756, #14756, #14756, #14756, #14756, #14756, #14756, #14756, #14756, #14756, #14756, #14756, #14756 Blocking:
Patch: yes

Description

I started implementing the left and right-aligned tabs (wxAUI_NB_LEFT|wxAUI_NB_RIGHT flags) and enhanced the bottom-aligned tabs on the wxAuiNotebook. It works pretty well but it is not yet finished.

I would like to know in particular what is the best management of the left-aligned tabs:

  • the first tab is at the bottom corner and the following at its right (like a counterclockwise rotation of 90° of the top-aligned tabs)
  • the first tab is at the top corner and the following at its left (the tabs appear in a reverse order)

The first solution is in my opinion the most readable if there are many tabs but if there are only one or two tabs it seems more interesting to see them on the top. What is your opinion?

Attachments (5)

wx_auinotebook_left_right-20120406-4.patch download (114.3 KB) - added by jens 5 years ago.
left and right alignment patch for auinotebook by Jens Lody
wx_auinotebook_left_right-20120413-1.patch download (117.6 KB) - added by jens 5 years ago.
wx_auinotebook_left_right_preparation.patch download (7.4 KB) - added by jens 5 years ago.
More exact determination, whether left/right buttons should be drawn.
wx_auinotebook_left_right_20120419-1.patch download (112.6 KB) - added by jens 5 years ago.
Cleaned up left and right alignment patch for auinotebook by Jens Lody
wxAuiNotebook_tab_alignment.patch download (57.6 KB) - added by R.U.10 5 years ago.
wxAuiNotebook wxAUI_NB_LEFT wxAUI_NB_RIGHT

Download all attachments as: .zip

Change History (90)

comment:1 Changed 6 years ago by vadz

  • Milestone 2.9.4 deleted

FWIW wxNotebook shows the first tab in the top left corner and the second one below it under both GTK and MSW so I think it would be logical to do this in AUI too.

comment:2 Changed 6 years ago by R.U.10

Ok, so I implemented a similar behavior to wxNotebook and updated the samples to play with the new functionalities.
I am waiting for your feedback.

comment:3 Changed 6 years ago by vadz

Thanks! As usual, I'm not the right person to review this patch as I hardly know wxAUI code but I had a look at it and noticed some things it would be nice to change:

  1. Could we make wxAUI_NB_XXX constants equal to wxBK_XXX counterparts? This would make most of the changes to the sample unnecessary.
  2. Minor: the "bit-wise or" (|) in #if wxUSE_AUI|wxUSE_NOTEBOOK test was probably supposed to be a normal "or" (||) as this is much more usual.
  3. Minor, related to the above: could we please use consistent spaces with bit operations? E.g. avoid things like m_flags &wxAUI_NB_LEFT and write m_flags & wxAUI_NB_LEFT instead?
  4. Testing for LEFT or RIGHT could be replaced with !IsVertical() test (this method is defined in the base wxBookCtrlBase class).
  5. Code exchanging x and y coordinates could be written as a single line using wxSwap().
  6. I wonder if wxMirrorDC could be used to avoid the code duplication here. Probably not but if it were possible, it would be great...

The last point is the most important one (although it would be nice to address the first one too), it really pains me to see so much new extra code mostly duplicating the existing functionality. wxAUI is already not exactly easy to maintain and adding several hundreds new lines of code is not going to help. It would be great if the code could be somehow refactored to avoid this.

comment:4 Changed 6 years ago by R.U.10

Thank you for your feedback.

I followed the points 1 to 5.
I can not use wxMirrorDC because of the light and shadow effects which cannot be mirrored.

I agree with you about the refactoring, I will try to take a look if I have a moment but not as part of this trac.

comment:5 Changed 5 years ago by jens

I started to implement the right and left alignment some days afte finishing the native tab art implementation for gtk.
After I had it working, I found this ticket (in fact I did not search before).
I did not test your patch against the actual trunk, because there are too many (incompatible) changes, like moving the tabart components in own files.

I decided not to start an own ticket, but to attach my patch here, after changing the wxAUI_NB_XXX enums as suggested by Vadim.

I cleaned up my patch, and think it's ready to be reviewed.

As far as I can see, it works correctly, without breaking the old interface.
So older self-developped implementations of wxAuiTabart-derivates should still work.

Changed 5 years ago by jens

left and right alignment patch for auinotebook by Jens Lody

comment:6 Changed 5 years ago by jens

  • Cc debian@… added

comment:7 Changed 5 years ago by jens

Is this ticket still alive, or better is anybody of the wx-devs taking notice of it ?
I don't want to start an extra thread in the mailing list, if not necessary.
But I would really like to here your opinion about the patches.

comment:8 Changed 5 years ago by vadz

  • Milestone set to 3.0
  • Status changed from new to confirmed

It's alive in the sense that my goal is to deal with all the patches sooner or later (and preferably sooner) but it's a huge patch affecting code I'm almost completely unfamiliar with so it's very difficult to review for me.

I'd appreciate reviews by others and feedback about testing it, ideally under different platforms.

TIA!

comment:9 Changed 5 years ago by jens

Thank you very much for your answer.

I tested my patch on linux (debian 64-bit, gcc 4.6) and windows 7 (64-bit), but with TDM's 32-bit MinGW 4.5 or 4.6.
I'm not sure about the compiler version on win and can not look at the moment, because my external hdd with all my vm's has crashed and it will take some time to recover it.

I might have the chance to get a Mac and tes it on Mac OS X 10.6 (Snow Leopard) the next days, but it will be the first time I develop on/for a Mac and it might take some time to make it work.

Jens

comment:10 Changed 5 years ago by mmacleod

Just a brief comment from this side, but the patch seems to have some unnecessary noise in it like adding of a new MyFrame::OnTabSplit function to the sample (that seems to be unrelated to this bug) and in some cases e.g. unnecessary whitespace changes or other unnecessary changes.

If you could possibly try and reduce as much noise from the patch as possible it might make it a bit easier to review...

comment:11 follow-up: Changed 5 years ago by mmacleod

Sorry, at a second look I see that the splitting is actually affected somewhat by these changes so presumably that is why it is also in the sample.
It would still be good however if any other unnecessary noise in the patch could be reduced.

comment:12 in reply to: ↑ 11 Changed 5 years ago by jens

Replying to mmacleod:

Sorry, at a second look I see that the splitting is actually affected somewhat by these changes so presumably that is why it is also in the sample.

That's correct:
I introduced the OnTabSplit function, to test whether it still works with left and right alignment.

It would still be good however if any other unnecessary noise in the patch could be reduced.

If I remove all whitespace changes, the formatting is broken (wrong indendation), I just ignore whitespace changes at eol at the moment.

The changes I made to chose the native tabart, if it exists (currently only wxGTK) and the generic tabart are also made to test whther the drawing routines for both tabarts work correctly on all platforms I can test.

I also changed some variable names, e.g. x_extent is now extent, because it can be either the extent in x- or y-direction.

comment:13 follow-up: Changed 5 years ago by mmacleod

I refer mostly to examples like the following:
rect.SetX(in_rect.x); -> rect.x = in_rect.x;
GetBestTabCtrlSize -> GetBestTabSize
m_flags -> m_windowStyle
if (m_flags & wxAUI_NB_BOTTOM) -> if (HasFlag(wxAUI_NB_BOTTOM))
(And various others)

While each of these individually might seem pretty harmless, and most of them can probably be argued in favour of, each instance of something like this creates a fair amount of extra lines that need to be examined when going over the patch which quickly adds up.
Worse from my perspective is that I have a large merge in the pipeline which make a large quantity of changes to e.g. wxAuiNotebook - so now it creates additional work where I must first go and make all these m_flags->m_windowStyle in the other branch as well if both this patch and the other branch are to be merged together.

Ideally it would be better if niceties like the above could be removed for now, and only the essentials left in.
A separate lower priority bug could then be perhaps opened for e.g. adding a HasFlag option and using m_windowStyle instead of m_flag, this lower priority bug would then be much easier to apply at a later date once all the more difficult (and important) code is merged.

I hope the above makes sense to you, I don't want to be difficult here and if this were the only large pending patch in question I probably wouldn't even bring it up - but I'm just trying to point out the fastest route forward given the current situation. If you can't do this I do of course understand but it would certainly make things easier if you could.

comment:14 in reply to: ↑ 13 Changed 5 years ago by jens

Replying to mmacleod:

I refer mostly to examples like the following:
rect.SetX(in_rect.x); -> rect.x = in_rect.x;

This can easily be changed (back) and can be pipelined for later to make the code more consistent.

GetBestTabCtrlSize -> GetBestTabSize

This is needed to keep backwards compatibility, or all other derived tabarts, that might exist in some apps (Code::Blocks uses own tabarts for example) will break.

m_flags -> m_windowStyle
if (m_flags & wxAUI_NB_BOTTOM) -> if (HasFlag(wxAUI_NB_BOTTOM))

These two are there because of point 4 in comment 4:

Testing for LEFT or RIGHT could be replaced with IsVertical() test (this method is defined in the base wxBookCtrlBase class).

IsVertical() uses HasFlag() and HasFlag uses m_windowStyle.
I can remove it for now and it can be probably changed later, if still wanted.

(And various others)

While each of these individually might seem pretty harmless, and most of them can probably be argued in favour of, each instance of something like this creates a fair amount of extra lines that need to be examined when going over the patch which quickly adds up.
Worse from my perspective is that I have a large merge in the pipeline which make a large quantity of changes to e.g. wxAuiNotebook - so now it creates additional work where I must first go and make all these m_flags->m_windowStyle in the other branch as well if both this patch and the other branch are to be merged together.

Ideally it would be better if niceties like the above could be removed for now, and only the essentials left in.
A separate lower priority bug could then be perhaps opened for e.g. adding a HasFlag option and using m_windowStyle instead of m_flag, this lower priority bug would then be much easier to apply at a later date once all the more difficult (and important) code is merged.

See above

I hope the above makes sense to you, I don't want to be difficult here and if this were the only large pending patch in question I probably wouldn't even bring it up - but I'm just trying to point out the fastest route forward given the current situation. If you can't do this I do of course understand but it would certainly make things easier if you could.

I can make these changes and post an updated patch the next days.
I will not be at home the next three days, but if I find the time to work on it tomorrow morning in the car, I can send it tomorrow evening.
If not I can bring up a new patch at latest at the weekend.

Jens

comment:15 Changed 5 years ago by mmacleod

This is needed to keep backwards compatibility, or all other derived tabarts, that might exist
in some apps (Code::Blocks uses own tabarts for example) will break.

Okay, that should obviously stay as is then.

IsVertical() uses HasFlag() and HasFlag uses m_windowStyle.
I can remove it for now and it can be probably changed later, if still wanted.

Hrm, I suppose given the above it better stay. Given that IsValid was implemented manually in two places I didn't realize that a derived version was also being used which makes the m_windowStyle change more necessary.

I can make these changes and post an updated patch the next days.

Okay, the ones I have mentioned are obviously just examples (and some of them seemingly poor ones at that) so please don't limit yourself to them, you would know better then myself which parts of your changes are necessary and which (if any) are just 'aesthetics' so if you could just try where it makes sense for now to separate out 'aesthetics' changes that would be great.

If not I can bring up a new patch at latest at the weekend.

The weekend would probably be great, thanks.

Changed 5 years ago by jens

More exact determination, whether left/right buttons should be drawn.

Changed 5 years ago by jens

Cleaned up left and right alignment patch for auinotebook by Jens Lody

comment:16 Changed 5 years ago by jens

I just attached two patches.
The first one is needed to determine, whether the left/right arrows should be drawn.
With just top and right alignment, it was not really a problem, because the last tab is usually large enough, that is not completely hidden before the forward/backward arrows are drawn. But with left and right alignment we have just the tabheight and if we have the "global" close-button or the windowlist-button shown, the last tab can be hidden, before the arrows are shown. So we will need a better detection of the visible space.

I made an extra patch of it, because it is not directly related to left and right alignment, but I did not open an extra ticket, because it needs to be applied for my second patch.

The second patch is still quite large, sorry.
Very few of the changes is not absolutely needed, like the change of some variable-names, but it makes no sense to have a variable named x_extent, if it is used for width and height, depending on the alignment, so I decided to keep these changes.

I removed the changes to the enums, that sync the auibook-enums with the bookctrl-enums.
But in my opinion this hould be applied sooner or later.

The attached patch just igores space-changes at EOL, I can also provide a patch, that ignores all space, but it's just 5 or 10 kB smaller and breaks the indendation of the code.

comment:17 follow-up: Changed 5 years ago by mmacleod

Okay, the patch mostly looks good to me.
I'm going to work on integrating it into the dynamic notebook branch and then assuming all goes well and I don't spot any problems while doing so will give it the green light to go into the main branch as well.
Will let you know if there are any specific queries that come up while i am applying the patch and looking at it in more detail, thanks.

comment:18 follow-up: Changed 5 years ago by mmacleod

Hello Jens,

On line 1702/1703 of auibook.cpp (line 1524 in trunk) you set m_tabCtrlHeight to 100, but the original line prior to that is not removed and that line sets the same variable to 20.
It isn't clear if (1) You intended to instead set width to 100? (2) You intended to remove the line that sets it to 20.
Can you please just verify this for me, thanks.

comment:19 in reply to: ↑ 18 Changed 5 years ago by jens

Replying to mmacleod:

Hello Jens,

On line 1702/1703 of auibook.cpp (line 1524 in trunk) you set m_tabCtrlHeight to 100, but the original line prior to that is not removed and that line sets the same variable to 20.
It isn't clear if (1) You intended to instead set width to 100? (2) You intended to remove the line that sets it to 20.
Can you please just verify this for me, thanks.

Ooops.
It should be the initial setting for the tab width (for left and right alignment):

        m_tabCtrlWidth = 100;

Jens

comment:20 in reply to: ↑ 17 Changed 5 years ago by jens

Replying to mmacleod:

Okay, the patch mostly looks good to me.
I'm going to work on integrating it into the dynamic notebook branch and then assuming all goes well and I don't spot any problems while doing so will give it the green light to go into the main branch as well.
Will let you know if there are any specific queries that come up while i am applying the patch and looking at it in more detail, thanks.

Hello,

I wanted to ask, whether you have made any progress with testing/integrating the patch ?

After the 2.9.4 release I started to add gtk3 support for wxAuiNotebook, but stopped it for the moment, because maintaining patches for it with and without left and right alignment is much more work.

Jens

comment:21 Changed 5 years ago by mmacleod

  • Blocked By 14756 added

comment:22 Changed 5 years ago by bpetty

  • Blocked By

(In #14756) I know I've mostly been absent from active development for a while here, but I'm going to try and at least help out with taking care of getting this branch merged in. This needs a lot of work on review and as many eyes as possible.

comment:23 Changed 5 years ago by mmacleod

  • Blocked By

comment:24 Changed 5 years ago by mmacleod

  • Blocked By

(In #14756) Oh yes one more thing, old 'aui' sample is left basically untouched in order to help test backwards compatibility, while 'auinew' makes use of all the new API functionality.

comment:25 Changed 5 years ago by vadz

  • Blocked By

(In #14756) OK, the changes here are too massive for any meaningful review (apparently a lot of stuff just got moved around but unfortunately it's not easy to see it in the diff), so I'll wait a couple of days for people to test this patch and complain about any breakage and just apply it if nobody does because it's this or just never merge it at all because I don't know of any reasonable way to review 20kLoC patch...

Please do test it if you use wxAUI!

comment:26 Changed 5 years ago by vadz

  • Blocked By

(In #14756) Unfortunately my first impressions are not good. At least under MSW there are pretty bad visual artefacts when dragging notebook pages, e.g. in the (old) aui sample I can see one page show through the other while dragging and after landing it sometimes I get another page displayed in the lower part of the page I dragged. There is also bad flicker when dragging which I think is new.

Notebook tabs on the left/right work but scrolling them doesn't, so some pages can become inaccessible. Also, the scrolling arrow doesn't seem to disappear once it has appeared, even if the notebook is resized to have enough space. But this is minor compared to the problem above.

Please let me know if you don't see this and I'll make some screenshots and describe the actions in more details but I think it should be easy to reproduce, there are pretty bad visual artefacts (again, at least under MSW with Windows 7, I didn't test elsewhere yet).

comment:27 Changed 5 years ago by jens

  • Blocked By

(In #14756) I will test it and try to find a solution (at least for the left-right related stuff, as this is based on a patch of mine).
Should I add patches here, or in the original thread ?

comment:28 Changed 5 years ago by vadz

  • Blocked By

(In #14756) Jens, are you using git too? If so, I think I could commit this branch into a github repository and you could work on it there (I know that Malcolm already has his repository on gitorious but it doesn't seem to be compatible with our official git mirror so I'd rather use that one). Because otherwise things risk getting very messy because I already did some local changes to the patch (mostly minor style things but some renamings too)...

comment:29 Changed 5 years ago by jens

  • Blocked By

(In #14756) Vadim, yes, I use git and git-svn.
If you can commit the branch to github it would be fine for me.

comment:30 Changed 5 years ago by vadz

  • Blocked By

(In #14756) OK, I've pushed my current version of the patch to https://github.com/vadz/wxWidgets/tree/aui-dynamic-notebook

comment:31 Changed 5 years ago by mmacleod

  • Blocked By

(In #14756) @vadz
Note that I've since uploaded a new version which fixes several issues that have been picked up, including (hopefully) most of the visual artifacts you mentioned. Not sure how we should proceed if you already have local patches as well? I'm busy setting up a new windows test VM and will have a look if I can find any remaining issues (on GTK at least everything now seems solid)

@jens
Thanks that will be a big help, let me know if you have any questions though, I'm also available on wxwidgets IRC channel if you want to discuss anything.

comment:32 Changed 5 years ago by mmacleod

  • Blocked By

(In #14756) Trying with the latest patch on an XP VM I do not see any of the visual glitches or artifacts, so I'm going to assume for now that the latest patch has fixed these. Please let me know if they are still occurring, I will try set up a win 7 box to test on as well.

Please ignore ( aui_dynamic_notebooks.patch.2.gz ) file - this was a mistake, I would delete it but there doesn't seem to be an option for that.

comment:33 Changed 5 years ago by vadz

  • Blocked By

(In #14756) Malcolm, sorry, I didn't see your other patch and now I'm stuck because I don't know what to do with this one as I don't have the original version of the patch any more. Could you please reupload the original one here so that I could apply the difference between two patches? Of course, if you can make a patch directly against the URL from comment:9 it would be perfect.

TIA!

comment:34 Changed 5 years ago by mmacleod

  • Blocked By

(In #14756) Okay, I've pulled your latest git and generated a patch between the two (above). Will do any further work against your git so that we don't have any more issues like this.

comment:35 Changed 5 years ago by vadz

  • Blocked By

(In #14756) It's still almost impossible to apply :-( The trouble is that it remodifies the things that I undid/removed from the original patch, e.g. all the extra white space additions and changes (" *" vs "* " -- it would be really better to avoid them) and renames back "wxAuiXXX" functions to "Aui_" ones.

I'm afraid the only solution is to make a diff against the original diff, otherwise I simply don't see what changed and can't apply it.

comment:36 Changed 5 years ago by mmacleod

  • Blocked By

(In #14756) Unfortunately I foolishly overwrote the original patch, so the best I can do is the above (new patch against older revision) which at least should make the changes clear.
Can you work with that?

Otherwise I will just quickly redo them against your branch, it is not a lot of changes.

comment:37 Changed 5 years ago by vadz

  • Blocked By

(In #14756) Yes, I should be able to apply this, thanks! I'll try to do this tonight.

I'll probably separate it into significant changes and the rest (e.g. "it's" vs "its"), hopefully this won't be a problem for you as you should be able to just start working on the github branch once this is applied.

comment:38 Changed 5 years ago by mmacleod

  • Blocked By

comment:39 Changed 5 years ago by mmacleod

  • Blocked By

comment:40 Changed 5 years ago by vadz

  • Blocked By

(In #14756) Thanks, I've applied the latest patch now and visually it's indeed much better, it still seems somewhat sluggish but at least it looks correctly, thanks for fixing this!

However I can trivially make it crash, e.g. by just dropping any notebook page on the title bar of the lower messages window with the following back trace:

 	auidemo.exe!wxBaseArrayPtrVoid::GetCount()  Line 832 + 0x11 bytes	C++
 	auidemo.exe!wxAuiTabContainer::SetActivePage(wxWindow * wnd=0x00aa28a0)  Line 976 + 0xb bytes	C++
>	auidemo.exe!wxAuiManager::SetActivePane(wxWindow * activePane=0x00aa28a0)  Line 757	C++
 	auidemo.exe!wxAuiManager::OnLeftUp(wxMouseEvent & evt={...})  Line 5843	C++

It seems that ctrl in wxAuiManager::SetActivePane() is garbage/uninitialized. Do we need to check for FindTab() return value perhaps?

comment:41 Changed 5 years ago by mmacleod

  • Blocked By

(In #14756) Hmm, no.
Although it is probably a good idea to check that anyway, but it isn't the root cause. (It will just not work correctly instead of crashing - which is of course an improvement)

The problem is that its trying to set an active pane when the wxAuiTabContainer does not yet contain that pane.
The latest patch is meant to fix that and I could have sworn it did in my branch, but perhaps I missed something. (Or maybe the patch missed something)
I'll take a look in the morning and try provide a more permanent fix for this.

comment:42 Changed 5 years ago by mmacleod

  • Blocked By

(In #14756) It seems that the code was getting a bit confused about external drags between managers (from the notebook manager to the surrounding aui manager) - even though the drop was being disallowed it was still going ahead and causing a crash.

The above patch improves the way external drags are handled and should fix the crash, let me know if it doesn't.
(Note the drag won't actually succeed in this case because the parent manager does not handle the wxEVT_AUI_ALLOW_DND event, but if you create a new notebook via the menu you should be able to drag externally between the two notebooks (The old sample implements wxEVT_AUI_ALLOW_DND only for notebooks)

comment:43 Changed 5 years ago by mmacleod

  • Blocked By

comment:44 Changed 5 years ago by vadz

  • Blocked By

(In #14756) Thanks, one question after looking at the patch: could we avoid the need for dynamic cast to wxAuiNotebook by always generating wxEVT_AUI_ALLOW_DND and then generating wxEVT_COMMAND_AUINOTEBOOK_ALLOW_DND from its default handler in wxAuiNotebook? If we could do it like this, it'd be much cleaner IMHO. But maybe I'm missing something that prevents us from doing it like this?

comment:45 Changed 5 years ago by mmacleod

  • Blocked By

(In #14756) Yes, I don't see anything stopping it from being done that way and I agree it would be cleaner.

comment:46 Changed 5 years ago by jens

  • Blocked By

(In #14756) First:
sorry for the delay.
Here is a first patch that fixes issues with left/right tabs.
Moving the tabs with the up-/down-arrows works, furthermore the up-arrow is visible again.

I implemented a short loop, that ensures we can see as much tabs as fit on the screen, or in other words: while there is enough free space on the right or bottom of the tab-control for a tab (and not all tabs are shown) the taboffset is decreased.
This option can possibly be made configurable with a flag.

I also change dthe default tab-height from 30 to 42, but that's just another hack until we can use calculated values again.

comment:47 Changed 5 years ago by Hanmac

  • Blocked By

comment:48 Changed 5 years ago by vadz

  • Blocked By

(In #14756) Thanks for the patches but I'm concerned about the use of wxDC::GetSize() as AFAIR it's not implemented in wxOSX. Why do we have to use it, don't we have the size in the window itself somewhere?

Also, the situation with variable names is completely catastrophic :-( We can't keep renaming them like this, with one patch doing 's/foo_bar/fooBar/g' and the next one doing 's/fooBar/foo_bar/g'. Ideal would be to keep the original names until the dust settles and then do one single renaming at the end. But this is going to be difficult as the changes of the original patch contained a lot of name changes.

BTW, speaking of this, I am really not sure we want to deprecate the old non m_-prefixed names of wxAuiPaneInfo fields. There is no real harm in not using m_ here and this results in hundreds of deprecation warnings, so I'll probably undo this before merging into the trunk.

comment:49 Changed 5 years ago by jens

  • Blocked By

(In #14756) About wxDC::GetSize(), yes it's needed at the moment, the window-size is unusable at the moment, but if it is really needed, I will find another solution.
As far as I know I only use it in wxAuiGtkTabArt, is wxGTK usable on Mac ? and if it is, wouldn't it use the same wxDC than on linux ?

About the variable names, where did the change from fooBar to foo_bar ?
I tried to keep them as they are in the initial merge-patch.
But it might be, that I have overread some.

comment:50 Changed 5 years ago by vadz

  • Blocked By

(In #14756) Replying to jens:

About wxDC::GetSize(), yes it's needed at the moment, the window-size is unusable at the moment

I don't really understand how can wxDC have the correct size if the window doesn't. Where does the DC size come from?

, but if it is really needed, I will find another solution.

As far as I know I only use it in wxAuiGtkTabArt, is wxGTK usable on Mac ?

Maybe/probably, but it's true that I overlooked the fact that it was only used in wxGTK-specific code so wxOSX considerations are irrelevant here. Still, I'm bothered by the fact that window size can't be used, it's probably another bug somewhere.

About the variable names, where did the change from fooBar to foo_bar ?

totWidth is being changed to tot_width in several places.

There are also not really necessarily changes of m_flags & XXX to HasFlag(XXX). It's not that I disagree that the latter is better but I'd really like to avoid making all these changes at the same time, it would be much better to leave them off until after the merge.

comment:51 Changed 5 years ago by jens

  • Blocked By

(In #14756) Replying to vadz:

Replying to jens:

About wxDC::GetSize(), yes it's needed at the moment, the window-size is unusable at the moment

I don't really understand how can wxDC have the correct size if the window doesn't. Where does the DC size come from?

, but if it is really needed, I will find another solution.

As far as I know I only use it in wxAuiGtkTabArt, is wxGTK usable on Mac ?

Maybe/probably, but it's true that I overlooked the fact that it was only used in wxGTK-specific code so wxOSX considerations are irrelevant here. Still, I'm bothered by the fact that window size can't be used, it's probably another bug somewhere.

The DrawButton seems to get the frame containing the whole notebook (including the pane) as parameter.
The dc is a memory dc especially created for drawing and it has the correct size.
Before the merge the window was the wxAuiTabCtrl, which no longer exists.
The rect can not be used directly, because the width is decreased for each drawn button, so the close and window-list button can be drawn at the right side, but the up button needs to be drawn in the middle of the rect, but this can not be determined after the width has decreased.
But I'm sure there will be another solurtion.

About the variable names, where did the change from fooBar to foo_bar ?

totWidth is being changed to tot_width in several places.

Ooops, sorry. I will change it back, or you can do it.

There are also not really necessarily changes of m_flags & XXX to HasFlag(XXX). It's not that I disagree that the latter is better but I'd really like to avoid making all these changes at the same time, it would be much better to leave them off until after the merge.

I did this, to be more consistent with other parts of the merge.
But I agree, it makes it harder to review the patches.

comment:52 Changed 5 years ago by jens

  • Blocked By

(In #14756) I fixed the sizing problems, or more exactly calculation of tabs (width and height) now works correctly.
Also the drawing of the buttons/arrows.

The tabart-class now has to care for the placement of the buttons and no longer the Render()-function, what was wrong in my opinion.
The tabart-class has to "decide" which buttons should be placed where.

Drawing of the right-tabs with gtk is also fixed.

Some minor issues are left with bottom tabs (at least with generic-tabart.
But this should not be a greatl problem.

Should I create a new patch based on the old ones (assuming the old patches have been applied) or should I create (a) new patch(es).

And what about the naming (camelCase vs. however_it_is_called) and using of HasFlag instead of m_flags&xxx ?
Shall I fix it also ?

I can upload (a) patch(es) this evening

comment:53 Changed 5 years ago by mmacleod

  • Blocked By

(In #14756) > BTW, speaking of this, I am really not sure we want to deprecate the old non m_-prefixed names of wxAuiPaneInfo fields. There is no real harm in not using m_ here and this results in hundreds of deprecation warnings, so I'll probably undo this before merging into the trunk.

I agree here that there is no need to rush the deprecation, feel free to take the deprecation away for now.

comment:54 Changed 5 years ago by vadz

  • Blocked By

(In #14756) Replying to jens:

I fixed the sizing problems, or more exactly calculation of tabs (width and height) now works correctly.
Also the drawing of the buttons/arrows.

Great, thanks!

Should I create a new patch based on the old ones (assuming the old patches have been applied) or should I create (a) new patch(es).

The currently applied patches are in the aui branch of git@…:vadz/wxWidgets.git

And what about the naming (camelCase vs. however_it_is_called) and using of HasFlag instead of m_flags&xxx ?
Shall I fix it also ?

I'd be really glad if all the patches could only include the important/critical changes right now and we postponed the rest of them until later. I'll actually probably try to revert some of the naming changes of the original patch just to be able to see more clearly what it does.

I can upload (a) patch(es) this evening

If you do, please let me know if they should be applied after aui_fix_external_tab_drop_crash2.patch or before it. Unfortunately I won't have time to apply this one today.

comment:55 Changed 5 years ago by jens

  • Blocked By

(In #14756) I just attached two patches, that (hopefully) fix almost all drawing issues in auinotebook.
You can apply them before or after the drop-crash patch, both works for me.

The calculate-tab-size patch must be applied before the fix-drawing-issues patch.

What stiil needs to be done is reimplementing the fixes for #14710.

That's the next thing I will do.

There are other sizing issues left, when draging (or better after dropping) tabs in some cases.
And I still can produce crashes on drop (even with the crash-drop-patch).

I will try to give detailed steps to reproduce, or find the reason.
But the drawing stuff has a higher priority for me, because I'm more familiar with the drawing code.

comment:56 Changed 5 years ago by mmacleod

  • Blocked By

(In #14756) Replying to jens:

I just attached two patches, that (hopefully) fix almost all drawing issues in auinotebook.
You can apply them before or after the drop-crash patch, both works for me.
The calculate-tab-size patch must be applied before the fix-drawing-issues patch.

Thanks, will try have a look over it this afternoon.

There are other sizing issues left, when draging (or better after dropping) tabs in some cases.

If you can let me know what cases I can have a look from this side as well.

And I still can produce crashes on drop (even with the crash-drop-patch).
I will try to give detailed steps to reproduce, or find the reason.

If you can give a stack race that should be enough, although any detailed steps are also appreciated.
Will have another look from this side as well.

But the drawing stuff has a higher priority for me, because I'm more familiar with the drawing code.

Yes, its probably more efficient if we each focus on the code we are more familiar with.

Thanks,
Malcolm

comment:57 Changed 5 years ago by jens

  • Blocked By

(In #14756) I just saw that fixed-size of tabs is broken.

There is one typo, but also a real issue with the order of calculating dynamic and fixed width and height.

I try to find a solution and post a patch.
Other stuff should not be affected.

Jens

comment:58 Changed 5 years ago by jens

  • Blocked By

(In #14756) Replying to jens:

I just saw that fixed-size of tabs is broken.

I just attached a patch that should fix this.

Jens

comment:59 Changed 5 years ago by jens

  • Blocked By

(In #14756) Hi Malcolm,

I just uploaded two backtraces after a drop-crash.
Your patch and my new patches are installed.

The first one appeared after a drop near the bottom of the auidemo window.
It happened sveral times, but I did not find a way to reproduce it reliably.
The scond one happened with close on all tabs active. I pressed the mouse over the close-button and dragged the tab, no hint-rectangle was visible before the crash.

Jens

comment:60 Changed 5 years ago by vadz

  • Blocked By

(In #14756) I applied (admittedly, without any review as I'm simply too lost here) all the patches to facilitate testing but the aui sample still crashes all the time. E.g. just make the tree pane floating -- it dies in wxAuiFloatingFrame::OnMoveFinished()...

comment:61 Changed 5 years ago by mmacleod

  • Blocked By

(In #14756) Okay thanks.
I've managed to reproduce the crash here and I should have some spare time tomorrow so will try get to the bottom of it and do a bit more stress testing.

Regarding the left/right changes I have a concern, for left/right notebooks "fixed width" seems to have been "re-purposed" to mean "fixed height".
I don't think this is a good idea, as it could be confusing for users. Left/Right notebooks are currently allocating as much space as necessary to fit the "page title" for very long page titles this can look somewhat unwieldy and as such it is not hard to imagine that a program author in this situation might want a "fixed width" for right/left tabs, getting instead a fixed height would be annoying.
Should we not consider just separating the two and having a "fixed height" as well, one could have both "fixed width" and "fixed height" for left/right notebooks, while for top/bottom notebooks "fixed height" could be ignored. (Unless of course some meaningful behavior could be done here as well)

comment:62 Changed 5 years ago by jens

  • Blocked By

(In #14756) At the moment "fixed height" for left/right is the same as "fixed width" for top/bottom: distribute the available space equally on all tabs, using a hard-coded min value.
Maybe wee need (or better should introduce) a min (and probably) a max-size for all alignments and also a real fixed width (or better size), which is not dynamical as it is now for top/bottom tabs.
SetMinSize(), SetMaxSize() and probaly a SetSize() could be used.
What we have now is more or less the same as sizeritems with wxEXPAND set, with a hardcoded min- and max-size and same proportion for all of them.

comment:63 Changed 5 years ago by mmacleod

  • Blocked By

(In #14756) Above patch should fix those two crashes.
The one fix is not 100% right as it fixes th symptom but not the underlying cause, I will need to look into it in a bit more detail to fix the underlying cause as well, will try do that this afternoon.
In the meantime would be good to know if this at least fixes the crashes for you or if they continue.

Thanks,
Malcolm

comment:64 Changed 5 years ago by mmacleod

  • Blocked By

(In #14756) Okay, updated version of the patch attached.

This properly fixes the cause of the above crashes as well as one or two other "strange" behaviors that were related. There seems to be one more crash lurking but it takes quite a while to reproduce and I've not yet managed to catch it in a debugger, will carry on looking into it but that should probably be applied separately (after this patch)

comment:65 Changed 5 years ago by jens

  • Blocked By

(In #14756) I attached three patches:

the first one reimplements the changes made for #14710,

the second fixes minor drawing issues in generic tabart,

the third fixes build issues I mentioned in windows 7:
auibook.h needs containr.h for wxNavigationEnabled and framemanager.cpp needs dcmemory.h for wxMemoryDC.

comment:66 Changed 5 years ago by jens

  • Blocked By

(In #14756) The newest patch implements a better calculation of tab- and gapbox-size.

comment:67 Changed 5 years ago by RedTide

  • Blocked By

(In #14756) I tested the auidemo under GTK2 and GTK3 applying also all the patches starting from aui_fix_tab_drops.patch,
let me know if this is correct (where is the new auidemo?).

I got some debug warnings under GTK2 and a crash on GTK3.

Both warnings and crash seems to happen in some circumstances when dragging some wxAuiToolBar control, where in GTK2 in particular, the vertical toolbar sometimes, trying to dock it when floated, disappears.
Other than that, when switching to the 'all panes' perspective, some toolbars shrinks to the left (same issue I have in my AUI XRC patch), so troubles (crash and warnings) starts when trying to move them (they seems to resize back to the original size once dragged).

The only weird thing I noticed (without considering the wxAuiToolBar issues) is the 'global' close button on the right, when no notebook tab have one, that disappears when changing perspective and go back again to the startup one, resulting with the original 'close button in all tabs' behaviour.

About the warnings I'm not sure if they are related to my GTK2 theme I'm using in my Ubuntu 12.10 + LXDE system, I have to test again applying changes in my local trunk.

comment:68 Changed 5 years ago by RedTide

  • Blocked By

(In #14756) > The only weird thing I noticed (without considering the wxAuiToolBar issues) is the 'global' close button on the right, when no notebook tab have one, that disappears when changing perspective and go back again to the startup one, resulting with the original 'close button in all tabs' behaviour.

Oops, this happens when changing notebook theme and default behaviour is 'close button in active tab'.

comment:69 Changed 5 years ago by jens

  • Blocked By

(In #14756) Replying to RedTide:

The only weird thing I noticed (without considering the wxAuiToolBar issues) is the 'global' close button on the right, when no notebook tab have one, that disappears when changing perspective and go back again to the startup one, resulting with the original 'close button in all tabs' behaviour.

Oops, this happens when changing notebook theme and default behaviour is 'close button in active tab'.

I think this is related to the changes of the constants for the auibook-flags in the dynamic aui branch.
Should be easily fixable.

In fact I saw it also, but it has low priority at the moment.

Gtk3 is more interestant, but drawing (and calculating of sizes) has changed a lot, so I still have no patch to attach here.
It compile sfine with gtk3 and drawing works also, but tabsize calculating still needs some more work.

comment:70 Changed 5 years ago by RedTide

  • Blocked By

(In #14756) Replying to jens:

Replying to RedTide:

Gtk3 is more interestant, but drawing (and calculating of sizes) has changed a lot, so I still have no patch to attach here.
It compile sfine with gtk3 and drawing works also, but tabsize calculating still needs some more work.

I think that this should be done later, if possible, in wxRenderer with a 'new' (wxUniv have it already) DrawTab().

comment:71 Changed 5 years ago by RedTide

  • Blocked By

(In #14756) I tested also on MSW and I got a segmentation fault adding a new notebook and then dragging a notebook page to the new one.
I'm using XP SP3 under VirtualBox + MinGW (official, not TDM) + MSYS (GCC 4.7.2).
It seems that gdb won't redirect output to a file like it does in linux, and interpret them as auidemo command line parameters, so, if needed, I can provide a jpg of the backtrace...

comment:72 Changed 5 years ago by vadz

  • Blocked By

(In #14756) Thanks for the testing. I still didn't have time to do much debugging of it myself but your results show that we still can't merge this in, getting crashes is really not acceptable.

So the question is whether we should delay 2.9.5 until somebody has time to fully iron out all the fatal bugs in this code or just go ahead with 2.9.5 and postpone this for 3.2.

Neither alternative is very appealing unfortunately :-(

comment:73 Changed 5 years ago by RedTide

  • Blocked By

(In #14756) Replying to vadz:

So the question is whether we should delay 2.9.5 until somebody has time to fully iron out all the fatal bugs in this code or just go ahead with 2.9.5 and postpone this for 3.2.

Neither alternative is very appealing unfortunately :-(

I don't see why a release of a development version shouldn't wait a bit, but this depends to how many hands/heads will work on this issue.
I'm for waiting this to be fixed and applied before 2.9.5, with some effort also from other devs also if not much interested on AUI.
If 2.9.5 will be last development version before 3.0 release, users will test it more deeply, but this is just my own opinion.

comment:74 Changed 5 years ago by jens

  • Blocked By

(In #14756) If this branch will not make it into 2.9.5 I would like to (back-)port the left-/right alignment stuff to trunk (in fact it was done already and should work without much effort).

It's a nice feature to have the tabs at the left or right side in times of wide-scale monitors.

There is already a ticket open (currently blocked by this one).
Last comment there was http://trac.wxwidgets.org/ticket/13787#comment:20 .

In any case we should decide what to do with the fixed-width/fixed-height(?) flag.

Currently it's more a give all tabs an equal width flag, which indeed not makes much sense at all and more or less no sense for left-/aright-alignment. Her we should have something like max width or make fixed-width a real fixed-width no matter what the content is.

comment:75 Changed 5 years ago by RedTide

  • Blocked By

(In #14756) Replying to jens:

If this branch will not make it into 2.9.5 I would like to (back-)port the left-/right alignment stuff to trunk (in fact it was done already and should work without much effort).

Is your GTK3 work to be going in wxRenderer?

I would like to see drawing stuff applied to it to having a native drawing apart, this could make code modular
and some parts also of this patch could be extracted and applied already and other non AUI users could benefit from it, but maybe I wrong/miss something.
Anyway I did some initial code to patch against wxRenderer also for tabs other than toolbars if it could be useful.

Vadim, could be possible to apply all the patches above and sync your clone with current trunk to be able to work on this issue meanwhile?

comment:76 Changed 5 years ago by vadz

  • Blocked By

(In #14756) Replying to RedTide:

Vadim, could be possible to apply all the patches above and sync your clone with current trunk to be able to work on this issue meanwhile?

I'm actually lost as to which patches should be applied (and in which order). I had started hacking on my own version about 3 weeks ago, trying to get rid of insignificant changes (and thus understand the significant ones better) but never finished it so right now my tree is a mess...

Anyhow, does anybody have a branch with all the patches applied to it on Github by chance? I think it would be more productive to work on this there instead of attaching patches to this ticket.

TIA!

comment:77 Changed 5 years ago by RedTide

  • Blocked By

(In #14756) Replying to vadz:

Replying to RedTide:

Vadim, could be possible to apply all the patches above and sync your clone with current trunk to be able to work on this issue meanwhile?

I'm actually lost as to which patches should be applied (and in which order). I had started hacking on my own version about 3 weeks ago, trying to get rid of insignificant changes (and thus understand the significant ones better) but never finished it so right now my tree is a mess...

Anyhow, does anybody have a branch with all the patches applied to it on Github by chance? I think it would be more productive to work on this there instead of attaching patches to this ticket.

TIA!

The order, AFAIU, is starting from aui_fix_tab_drops.patch, that seems replacing the previous one (I had to add a --ignore-whitespaces to apply) and below.

I have it at home with all the patch applied, so I could, if Malcom and Jens agree, upload it in my Github account and take care to sync it with the main upstream adding you all with write permission after uploading (merge the current trunk to it after upload as first commit, using it as origin where you will work on and adding the read only wx trunk as upstream to be able to keep this branch in sync with it).
I can do it only starting from the next week so meanwhile let me know if this could be a useful/correct solution.

comment:78 Changed 5 years ago by vadz

  • Blocked By

(In #14756) It would be definitely useful, thanks.

But I'm still tempted to release 2.9.5 now because I didn't manage to work on this this weekend (got ill and spent what little time I managed on fixing a couple of new bad regressions instead) and so we're basically not going to release 2.9.5 at all this year if we delay it further.

We could do a 2.9.6 early in the next year if this stabilizes enough for then though.

comment:79 Changed 5 years ago by RedTide

  • Blocked By

(In #14756) I've pushed the changes to https://github.com/redtide/wxWidgets/tree/aui-dynamic-notebook
against the current trunk code:
after some talking with Brian, I realized that should be better that anyone have to keep his own wxWidgets forked repo and pull only from other ones instead sharing the same, so I hope that my simple file replacement with the files with all patches applied is fine too.
Let me know about any problem/issue, meanwhile I'll test it.

comment:80 Changed 5 years ago by vadz

  • Blocked By

(In #14756) We need to integrate r73263 (see #10848) into this branch,

comment:81 Changed 5 years ago by RedTide

  • Blocked By

(In #14756) Replying to vadz:

We need to integrate r73263 (see #10848) into this branch,

I suppose also r73288 / #14927
I think I'll wait for Malcom before do this my self on my cloned repo.

comment:82 Changed 5 years ago by RedTide

  • Blocked By

(In #14756) Replying to RedTide:

Replying to vadz:

We need to integrate r73263 (see #10848) into this branch,

I suppose also r73288 / #14927
I think I'll wait for Malcom before do this my self on my cloned repo.

I'm not familiar with this code, but it is so different that I suppose that both issues was already managed, so I merged the code with a recent trunk version leaving auibook.cpp code unmodified.

comment:83 Changed 5 years ago by vadz

  • Blocked By

(In #14756) Sorry, this clearly won't happen for 3.0 :-(

comment:84 Changed 5 years ago by R.U.10

FWIW, I updated my patch related to the changes in r71001. I did not work on the file tabartgtk.cpp inserted in r71002 so I guess the patch would work only on MSW.

Changed 5 years ago by R.U.10

wxAuiNotebook wxAUI_NB_LEFT wxAUI_NB_RIGHT

comment:85 Changed 4 years ago by vadz

  • Milestone changed from 3.0 to 3.2

All wxAUI issues will have to wait until we deal with the question of its refactoring/rewrite/replacement and this won't happen before 3.0.

Note: See TracTickets for help on using tickets.