Jean-Marc Lasgouttes wrote:
>
> Peter> Here a patch to simplify the tabbar code. As JMarc suggested
> Peter> the tabbars are always build from scratch, when the buffer has
> Peter> changed.
>
> I have some remarks about your patch. I think the code can be further
> simplified.
I'm glad not having checked it in ;)
> Index: src/frontends/qt4/GuiView.C
> ===================================================================
> --- src/frontends/qt4/GuiView.C (revision 16981)
> +++ src/frontends/qt4/GuiView.C (working copy)
> @@ -72,42 +72,43 @@
>
> namespace {
>
> -int const statusbar_timer_value = 3000;
> + int const statusbar_timer_value = 3000;
>
> The code into namespaces is not indented in our style. (below too).
>
> + // avoid unneeded tabbar rebuild: check if something has changed
> + if (oldnames.size() == names.size()) {
> + bool equal = true;
> + for (size_t i = 0; i < names.size(); i++) {
> + if (names[i] != oldnames[i]) {
> + equal = false;
> + break;
> + }
> + }
> + if (equal) {
> + return;
> + }
> + }
> + oldnames = names;
>
> Can't you just test oldnames == names ?
Ah, yes! I forgot that this is maybe supported by stl, if yes, then this is
better.
>
> Did you check what happens if you do not do this test? Is there any
> noticeable flicker or slowdown?
Yes, I always the the busy-mouse-symbol on windows.
>
> + // remove all tab bars and clear the function map
> + d.funcmap.clear();
> + for (int i = tabbar.count()-1; i >= 0; i--) {
> + tabbar.removeTab(i);
> }
>
> Can't you just create a new tabbar? It looks strange to have to remove
> elements one by one (why isn't there a QTabBar::clear function?)
"why isn't there a QTabBar::clear function?" therefore you have to ask
Trolltech.
But maybe I could replace the tabbar with a new one, have to check if this is
possible.
>
> Also, be careful about spacing (" - 1").
Do you talk about the formatting: (" - 1")?
>
> + // rebuild tabbar an function map from scratch
> + if (names.size() == 1) {
> + d.funcmap.insert(std::pair<int, FuncRequest>
> + (0,
> FuncRequest(LFUN_BUFFER_SWITCH, names[0])));
> + } else {
> + for(size_t i = 0; i < names.size(); i++) {
> + tabbar.addTab(lyx::toqstr(onlyFilename(names[i])));
> + d.funcmap.insert(std::pair<int, FuncRequest>
> + (i,
> FuncRequest(LFUN_BUFFER_SWITCH, names[i])));
> }
> Instead of using this funcmap, you should use lyx::Action like in the
> menu or toolbar code. This would avoid yet another extra structure.
I don't know this code.
>
> + // set current tab
> + if (view()->buffer()) {
> + string cur_title = view()->buffer()->fileName();
> + for(size_t i = 0; i < names.size(); i++) {
> + if (names[i] == cur_title) {
> + tabbar.setCurrentIndex(i);
>
> Why not do that in the previous loop?
Yes, why not.
>
> JMarc
>
--
Peter Kümmel
