[rekonq] Review Request: Refactoring of some part of the tab bar

Thu Aug 16 12:18:54 UTC 2012

> On Aug. 14, 2012, 4:12 p.m., Andrea Diamantini wrote:
> > Merged all commits but the last one. In fact the change to let tabs have different dimensions seems to me unnecessary (at least on 1.1) and an "exaggeration" in terms of configurability. If you'd like to go on that road, I see as better option (in example) the ability to enlarge the selected tab. Uhm... well. I don't really know. I'd like to hear others opinion about.

Yes in fact, I agree with you, maybe it's too configurable as there are already a lot of options in the "Tab" section.

Anyway, the main goal of this commit was to get rid of MainView::sizeHint() to compute a tab width.
So first, we should decide on how to compute that.
IMHO a fixed size (~200-250) is the best as it will fix all problem we have with KMessageWidget. But an option to configure that is needed, at least for the default size.
What do you think about that ?

- Yoann

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105938/#review17399
-----------------------------------------------------------

On Aug. 8, 2012, 9:43 p.m., Yoann Laissus wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105938/
> -----------------------------------------------------------
> 
> (Updated Aug. 8, 2012, 9:43 p.m.)
> 
> 
> Review request for rekonq.
> 
> 
> Description
> -------
> 
> The branch is here : http://github.com/Arakmar/rekonq/tree/tabBarRefactoring
> 
> The original goal of this merge request was to (finally) fix all problems we have with the interaction between the tabbar and KMessageWidget.
> While investigating for this problem, I also cleaned and fixed some bugs with the add tab button and tab previews which was hidden by the previous implementation
> That's why I decided to split that in independant parts. Only the last one depends on the first two. Those two parts can be safely backported in the 1.0 branch as it's only fixes.
> 
> First part : add tab button refactoring (6f291145ba8a4)
> This commit clean the updateTabBar method by splitting it in two methods (updateAddTabButton and updateTabBarVisibility) as it's two things very different.
> The position of the new tab button is now updated only on layout change of the tab bar.
> This method has the benefit to not call the slot in every method which add a tab.
> It also fixes bug 304325 and some nasty bugs with KMessageWidget due to non updated position.
> 
> Second part : Tab preview refactoring (b83d6a, 7e3158, f5f69)
> This part is mainly here to prepare the last one.
> Those commits introduce a fixed size (200px) for tab previews to fix issues with too big preview on very high ratio. (eg : preview of tabs for a 200x900 window)
> 200 can be discussed but it's the default size of a tab. (The size hint of the mainview is always the same, even on a resolution change ...)
> 
> Third part : Tab width refactoring (2cf613)
> The problem is described here :
> https://git.reviewboard.kde.org/r/102651/
> 
> When a KTabWidget appears, the tab width grows up. And without the first part of this MR, the add tab button isn't even updated.
> 
> The previous fix has been removed by some refactoring of Application and MainWindow.
> But I think we can't rely on this solution anymore, it's not the proper way to fix that.
> 
> That's why I have replaced the existing solution by a more configurable one.
> There is now the choice between two options (in the Tab section) :
> 
> - Fixed configurable size (Default to 200 and minimum 100)
> It's the replacement of the previous behaviour.
> We can't rely on MainView::tabSizeHint() as it's the same value no matter the resolution of the screen.
> Moreover, all major browsers use a fixed (configurable) size (Firefox - 250 px, Chrome - 200 px)
> So I think it's a lot safer to use that.
> 
> - Automatic size depending on the title size:
> It's the default behaviour of KTabBar (Konqueror)
> 
> (Sorry for this big description :D )
> 
> 
> This addresses bug 304325.
>     /show_bug.cgi?id=304325
> 
> 
> Diffs
> -----
> 
>   src/application.cpp 2127ca9 
>   src/mainview.h 9378f4b 
>   src/mainview.cpp 861a6b1 
>   src/rekonq.kcfg 1fb3c1c 
>   src/settings/settings_tabs.ui 4e19545 
>   src/settings/tabswidget.h 056f030 
>   src/settings/tabswidget.cpp 67829f8 
>   src/tabbar.h 161612c 
>   src/tabbar.cpp ac257d1 
>   src/tabpreviewpopup.h 5c62102 
>   src/tabpreviewpopup.cpp 63ff542 
> 
> Diff: http://git.reviewboard.kde.org/r/105938/diff/
> 
> 
> Testing
> -------
> 
> Tested on multiple computers with different resolutions.
> 
> 
> Screenshots
> -----------
> 
> Automatic width
>   http://git.reviewboard.kde.org/r/105938/s/665/
> Fixed width
>   http://git.reviewboard.kde.org/r/105938/s/666/
> Tab settings
>   http://git.reviewboard.kde.org/r/105938/s/667/
> 
> 
> Thanks,
> 
> Yoann Laissus
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/rekonq/attachments/20120816/b48ff72a/attachment.html>