Details

Reviewers

hein
alexeymin

Summary

Added tabbar scrolling support when the selected button cannot fit in the tabbar.
Changed the way that the tabbar is getting drawn in order to support a scrollbar in the future. Now the buttons in the tabbar are all getting drawn in an off-screen private pixmap, called the buttonBar, and the buttonBar is getting scrolled as needed and painted to the widget at once.

Added two more source files, the genericfuncs.cpp and genericfuncs.h, with generic functions for easy pixmap manipulation while drawing the off-screen buttons.
The newly added functions are the following:

const QPixmap drawPixmapOnPixmap(int x, int y, const QPixmap& pixmapUnder, const QPixmap& pixmapOver)
const QPixmap drawTextOnPixmap(int x, int y, int w, int h, int flags, const QFont& font, const QColor& textColor, const QString& text, const QPixmap& pixmap)
const QPixmap genHorizontallyTiledPixmap(const QPixmap& pixmap, uint repeatTimes)
const QPixmap appendPixmapToPixmapHorizontally(const QPixmap& pixmap1, const QPixmap& pixmap2)

This diff is related to the bug 339139, and I tried to implement the feature according to Eike's comment (https://bugs.kde.org/show_bug.cgi?id=339139#c3).

Tried to keep the changes to the minimum for this initial commit, and things can actually now be improved when compared to the past. For example, since the buttonBar is a global private variable now, we do not have to re-draw it from scratch everytime the paintEvent function is called. Only when a new button is added or removed, or when the user selects a different session (and therefore, another tabbar button has to be highlighted).

I am also working on a skin-able scrollbar at the moment (slowly).

Test Plan

Just open more tabs than the ones that can fit in the yakuake window. Scroll between them, close random tabs.
I have also tested the new scroll tab feature in a dual monitor setup, where the two monitors have different resolutions (thus, yakuake is resizing itself when changing monitor).

Diff Detail

Repository

R369 Yakuake

Branch

scroll-tabs

Lint

No Linters Available

Unit

No Unit Test Coverage

vtasoulas created this revision.Sep 7 2017, 8:28 PM

vtasoulas retitled this revision from Added tabbar scrolling support when the selected button cannot fit in the tabbar. to Bug 339139: Added tabbar scrolling support when the selected button cannot fit in the tabbar..

vtasoulas added a reviewer: KDE Applications.Sep 10 2017, 6:51 PM

Tested, it works ok! UI is not so obvious without a scroll bar, but... Diff has a lot of code already :)

Yes, lots of code already so let's see if this gets accepted first.
Before scrollbar comes in the UI, I see two more steps:

Improve the way the tabbar is getting generated. Now the tabbar is redrawn from scratch each time the painEvent is called (basically, every time the yakuake window appears/drops-down). That's how it was implemented and I didn't change that behavior in order to keep the commit to the minimum. The tabbar should only be generated when a new tab is added, removed or selected, and the paintEvent should only scroll and draw the cached m_ButtonBar pixmap as needed.
Introduce a scrollbar that is able to scroll the tabbar, but at the same time, some additional changes must be made to the tabbar because whenever tabs are added or removed, the size/position of the scrollbar may have to updated as well. A bidirectional interaction between the two widgets is needed here (the tabbar and the tabscroll widget).

Maybe instead of scrollbar, two buttons with arrow-left and arrow-right at the sides of the tabbar would be enough? like this (paint master!)

We don't have this now, right?
It will needs new icons for all skins... maybe ask VDG ?

No, we don't have tabbar arrows at the moment.

The simplest/quickest implementation that would not add much extra code at the moment, would be to add two arrows that would change the selected session (when clicking the left arrow, select the current - 1 session. The right arrow selects the current + 1 session) and not scroll the tabbar itself.
Of course, it will be a little bit annoying to scroll through, e.g. 15 tabs, as you would need to click 15 times on the arrows. But at least, there will be some indication that scrolling support is there.

My idea is that later on, the scrollbar should scroll the tabbar without changing the selected tab. In that way, the user could use the scrollbar to look through all of the available tabs and would have to click to another tab to select that session.

Added scroll buttons in the tabbar as suggested by @alexeymin.
Added support for the tabbar scroll buttons in the default (Breeze) theme.

This quite large commit is broken in three commits in the repository here: https://github.com/cyberang3l/yakuake/tree/scroll-tabs

vtasoulas added a reviewer: alexeymin.Oct 7 2017, 11:59 PM

@hein since this is a quite large commit that adds a new feature, I guess it is you who should decide if the implementation is acceptable or needs any modifications.

I tested the latest change and it does not work for me, no icons, no buttons, .. :(

Are you sure that the updated default skin is used?

If yakuake is using the default skin that is installed in the system already (in KDE Neon /usr/share/yakuake/skins/default/), then you will not see the arrows.

This is how the updated default skin should look like:

Another thing that I paid attention with this implementation is to not break existing skins. @alexeymin, as you might have noticed (hopefully if everything worked as intended) even if you couldn't see the arrow buttons with your selected skin, the skin looked as it used to. Moreover, the scrolling works in old skins, e.g. by using the mouse wheel button, even if there are no clickable arrow buttons.

Of course, if we are all satisfied with the changes, we have to update at least the skins that are shipped together with yakuake (but this should be quite simple) to contain arrow buttons.

@hein any possibility to get this merged? If not, any requested suggestions for improvements?

I tried once more; literally fresh checkout of yakuake.git, arc patch D7727, re-run cmake, make, make install, /usr/bin/yakuake and I see this:

I can clearly see that image files are installed, from make install output:

make[1]: Leaving directory '/home/lexx/dev/yakuake/build'
Install the project...
/usr/bin/cmake -P cmake_install.cmake
-- Install configuration: "RelWithDebInfo"
-- Installing: /usr/bin/yakuake
-- Installing: /etc/xdg/yakuake.knsrc
-- Installing: /usr/share/applications/org.kde.yakuake.desktop
-- Installing: /usr/share/metainfo/org.kde.yakuake.appdata.xml
-- Installing: /usr/share/knotifications5/yakuake.notifyrc
-- Installing: /usr/share/icons/hicolor/128x128/apps/yakuake.png
-- Installing: /usr/share/icons/hicolor/16x16/apps/yakuake.png
-- Installing: /usr/share/icons/hicolor/22x22/apps/yakuake.png
-- Installing: /usr/share/icons/hicolor/256x256/apps/yakuake.png
-- Installing: /usr/share/icons/hicolor/32x32/apps/yakuake.png
-- Installing: /usr/share/icons/hicolor/48x48/apps/yakuake.png
-- Installing: /usr/share/icons/hicolor/64x64/apps/yakuake.png
-- Installing: /usr/share/yakuake/skins/README
-- Installing: /usr/share/yakuake/skins/default/title.skin
-- Installing: /usr/share/yakuake/skins/default/tabs.skin
-- Installing: /usr/share/yakuake/skins/default/icon.png
-- Installing: /usr/share/yakuake/skins/default/tabs/add_down.png
-- Installing: /usr/share/yakuake/skins/default/tabs/add_down.svg
-- Installing: /usr/share/yakuake/skins/default/tabs/add_up.png
-- Installing: /usr/share/yakuake/skins/default/tabs/add_up.svg
-- Installing: /usr/share/yakuake/skins/default/tabs/back_image.png
-- Installing: /usr/share/yakuake/skins/default/tabs/back_image.svg
-- Installing: /usr/share/yakuake/skins/default/tabs/close_down.png
-- Installing: /usr/share/yakuake/skins/default/tabs/close_down.svg
-- Installing: /usr/share/yakuake/skins/default/tabs/close_up.png
-- Installing: /usr/share/yakuake/skins/default/tabs/close_up.svg
-- Installing: /usr/share/yakuake/skins/default/tabs/left_corner.png
-- Up-to-date: /usr/share/yakuake/skins/default/tabs/left_corner.svg
-- Installing: /usr/share/yakuake/skins/default/tabs/lock.png
-- Installing: /usr/share/yakuake/skins/default/tabs/lock.svg
-- Installing: /usr/share/yakuake/skins/default/tabs/right_corner.png
-- Up-to-date: /usr/share/yakuake/skins/default/tabs/right_corner.svg
-- Installing: /usr/share/yakuake/skins/default/tabs/selected_back.png
-- Installing: /usr/share/yakuake/skins/default/tabs/selected_back.svg
-- Installing: /usr/share/yakuake/skins/default/tabs/selected_left.png
-- Up-to-date: /usr/share/yakuake/skins/default/tabs/selected_left.svg
-- Installing: /usr/share/yakuake/skins/default/tabs/selected_right.png
-- Up-to-date: /usr/share/yakuake/skins/default/tabs/selected_right.svg
-- Installing: /usr/share/yakuake/skins/default/tabs/separator.png
-- Installing: /usr/share/yakuake/skins/default/tabs/separator.svg
-- Installing: /usr/share/yakuake/skins/default/tabs/unselected_back.png
-- Installing: /usr/share/yakuake/skins/default/tabs/unselected_back.svg
-- Installing: /usr/share/yakuake/skins/default/tabs/unselected_left.png
-- Up-to-date: /usr/share/yakuake/skins/default/tabs/unselected_left.svg
-- Installing: /usr/share/yakuake/skins/default/tabs/unselected_right.png
-- Up-to-date: /usr/share/yakuake/skins/default/tabs/unselected_right.svg
-- Installing: /usr/share/yakuake/skins/default/tabs/left_down.png
-- Installing: /usr/share/yakuake/skins/default/tabs/left_down.svg
-- Installing: /usr/share/yakuake/skins/default/tabs/left_up.png
-- Installing: /usr/share/yakuake/skins/default/tabs/left_up.svg
-- Installing: /usr/share/yakuake/skins/default/tabs/right_down.png
-- Installing: /usr/share/yakuake/skins/default/tabs/right_down.svg
-- Installing: /usr/share/yakuake/skins/default/tabs/right_up.png
-- Installing: /usr/share/yakuake/skins/default/tabs/right_up.svg
-- Installing: /usr/share/yakuake/skins/default/title/back.png
-- Installing: /usr/share/yakuake/skins/default/title/back.svg
-- Installing: /usr/share/yakuake/skins/default/title/config_down.png
-- Installing: /usr/share/yakuake/skins/default/title/config_down.svg
-- Installing: /usr/share/yakuake/skins/default/title/config_up.png
-- Installing: /usr/share/yakuake/skins/default/title/config_up.svg
-- Installing: /usr/share/yakuake/skins/default/title/focus_down.png
-- Installing: /usr/share/yakuake/skins/default/title/focus_down.svg
-- Installing: /usr/share/yakuake/skins/default/title/focus_over.png
-- Installing: /usr/share/yakuake/skins/default/title/focus_over.svg
-- Installing: /usr/share/yakuake/skins/default/title/focus_up.png
-- Installing: /usr/share/yakuake/skins/default/title/focus_up.svg
-- Installing: /usr/share/yakuake/skins/default/title/left.png
-- Installing: /usr/share/yakuake/skins/default/title/left.svg
-- Installing: /usr/share/yakuake/skins/default/title/quit_down.png
-- Installing: /usr/share/yakuake/skins/default/title/quit_down.svg
-- Installing: /usr/share/yakuake/skins/default/title/quit_up.png
-- Installing: /usr/share/yakuake/skins/default/title/quit_up.svg
-- Installing: /usr/share/yakuake/skins/default/title/right.png
-- Installing: /usr/share/yakuake/skins/default/title/right.svg
-- Installing: /usr/share/yakuake/skins/legacy/title.skin
-- Installing: /usr/share/yakuake/skins/legacy/tabs.skin
-- Installing: /usr/share/yakuake/skins/legacy/icon.png
-- Installing: /usr/share/yakuake/skins/legacy/tabs/back_image.png
-- Installing: /usr/share/yakuake/skins/legacy/tabs/left_corner.png
-- Installing: /usr/share/yakuake/skins/legacy/tabs/minus_down.png
-- Installing: /usr/share/yakuake/skins/legacy/tabs/minus_over.png
-- Installing: /usr/share/yakuake/skins/legacy/tabs/minus_up.png
-- Installing: /usr/share/yakuake/skins/legacy/tabs/plus_down.png
-- Installing: /usr/share/yakuake/skins/legacy/tabs/plus_over.png
-- Installing: /usr/share/yakuake/skins/legacy/tabs/plus_up.png
-- Installing: /usr/share/yakuake/skins/legacy/tabs/right_corner.png
-- Installing: /usr/share/yakuake/skins/legacy/tabs/selected_back.png
-- Installing: /usr/share/yakuake/skins/legacy/tabs/selected_left.png
-- Installing: /usr/share/yakuake/skins/legacy/tabs/selected_right.png
-- Installing: /usr/share/yakuake/skins/legacy/tabs/separator.png
-- Installing: /usr/share/yakuake/skins/legacy/tabs/unselected_back.png
-- Installing: /usr/share/yakuake/skins/legacy/tabs/lock.png
-- Installing: /usr/share/yakuake/skins/legacy/title/back.png
-- Installing: /usr/share/yakuake/skins/legacy/title/config_down.png
-- Installing: /usr/share/yakuake/skins/legacy/title/config_over.png
-- Installing: /usr/share/yakuake/skins/legacy/title/config_up.png
-- Installing: /usr/share/yakuake/skins/legacy/title/focus_down.png
-- Installing: /usr/share/yakuake/skins/legacy/title/focus_over.png
-- Installing: /usr/share/yakuake/skins/legacy/title/focus_up.png
-- Installing: /usr/share/yakuake/skins/legacy/title/left.png
-- Installing: /usr/share/yakuake/skins/legacy/title/quit_down.png
-- Installing: /usr/share/yakuake/skins/legacy/title/quit_over.png
-- Installing: /usr/share/yakuake/skins/legacy/title/quit_up.png
-- Installing: /usr/share/yakuake/skins/legacy/title/right.png
-- Installing: /usr/share/yakuake/skins/plastik_light/title.skin
-- Installing: /usr/share/yakuake/skins/plastik_light/tabs.skin
-- Installing: /usr/share/yakuake/skins/plastik_light/icon.png
-- Installing: /usr/share/yakuake/skins/plastik_light/tabs/back_image.png
-- Installing: /usr/share/yakuake/skins/plastik_light/tabs/left_corner.png
-- Installing: /usr/share/yakuake/skins/plastik_light/tabs/minus_down.png
-- Installing: /usr/share/yakuake/skins/plastik_light/tabs/minus_over.png
-- Installing: /usr/share/yakuake/skins/plastik_light/tabs/minus_up.png
-- Installing: /usr/share/yakuake/skins/plastik_light/tabs/plus_down.png
-- Installing: /usr/share/yakuake/skins/plastik_light/tabs/plus_over.png
-- Installing: /usr/share/yakuake/skins/plastik_light/tabs/plus_up.png
-- Installing: /usr/share/yakuake/skins/plastik_light/tabs/right_corner.png
-- Installing: /usr/share/yakuake/skins/plastik_light/tabs/selected_back.png
-- Installing: /usr/share/yakuake/skins/plastik_light/tabs/selected_left.png
-- Installing: /usr/share/yakuake/skins/plastik_light/tabs/selected_right.png
-- Installing: /usr/share/yakuake/skins/plastik_light/tabs/separator.png
-- Installing: /usr/share/yakuake/skins/plastik_light/tabs/unselected_back.png
-- Installing: /usr/share/yakuake/skins/plastik_light/tabs/lock.png
-- Installing: /usr/share/yakuake/skins/plastik_light/title/back.png
-- Installing: /usr/share/yakuake/skins/plastik_light/title/config_down.png
-- Installing: /usr/share/yakuake/skins/plastik_light/title/config_over.png
-- Installing: /usr/share/yakuake/skins/plastik_light/title/config_up.png
-- Installing: /usr/share/yakuake/skins/plastik_light/title/focus_down.png
-- Installing: /usr/share/yakuake/skins/plastik_light/title/focus_over.png
-- Installing: /usr/share/yakuake/skins/plastik_light/title/focus_up.png
-- Installing: /usr/share/yakuake/skins/plastik_light/title/left.png
-- Installing: /usr/share/yakuake/skins/plastik_light/title/quit_down.png
-- Installing: /usr/share/yakuake/skins/plastik_light/title/quit_over.png
-- Installing: /usr/share/yakuake/skins/plastik_light/title/quit_up.png
-- Installing: /usr/share/yakuake/skins/plastik_light/title/right.png
-- Installing: /usr/share/yakuake/skins/plastik_dark/title.skin
-- Installing: /usr/share/yakuake/skins/plastik_dark/tabs.skin
-- Installing: /usr/share/yakuake/skins/plastik_dark/icon.png
-- Installing: /usr/share/yakuake/skins/plastik_dark/tabs/back_image.png
-- Installing: /usr/share/yakuake/skins/plastik_dark/tabs/left_corner.png
-- Installing: /usr/share/yakuake/skins/plastik_dark/tabs/minus_down.png
-- Installing: /usr/share/yakuake/skins/plastik_dark/tabs/minus_over.png
-- Installing: /usr/share/yakuake/skins/plastik_dark/tabs/minus_up.png
-- Installing: /usr/share/yakuake/skins/plastik_dark/tabs/plus_down.png
-- Installing: /usr/share/yakuake/skins/plastik_dark/tabs/plus_over.png
-- Installing: /usr/share/yakuake/skins/plastik_dark/tabs/plus_up.png
-- Installing: /usr/share/yakuake/skins/plastik_dark/tabs/right_corner.png
-- Installing: /usr/share/yakuake/skins/plastik_dark/tabs/selected_back.png
-- Installing: /usr/share/yakuake/skins/plastik_dark/tabs/selected_left.png
-- Installing: /usr/share/yakuake/skins/plastik_dark/tabs/selected_right.png
-- Installing: /usr/share/yakuake/skins/plastik_dark/tabs/separator.png
-- Installing: /usr/share/yakuake/skins/plastik_dark/tabs/unselected_back.png
-- Installing: /usr/share/yakuake/skins/plastik_dark/tabs/lock.png
-- Installing: /usr/share/yakuake/skins/plastik_dark/title/back.png
-- Installing: /usr/share/yakuake/skins/plastik_dark/title/config_down.png
-- Installing: /usr/share/yakuake/skins/plastik_dark/title/config_over.png
-- Installing: /usr/share/yakuake/skins/plastik_dark/title/config_up.png
-- Installing: /usr/share/yakuake/skins/plastik_dark/title/focus_down.png
-- Installing: /usr/share/yakuake/skins/plastik_dark/title/focus_over.png
-- Installing: /usr/share/yakuake/skins/plastik_dark/title/focus_up.png
-- Installing: /usr/share/yakuake/skins/plastik_dark/title/left.png
-- Installing: /usr/share/yakuake/skins/plastik_dark/title/quit_down.png
-- Installing: /usr/share/yakuake/skins/plastik_dark/title/quit_over.png
-- Installing: /usr/share/yakuake/skins/plastik_dark/title/quit_up.png
-- Installing: /usr/share/yakuake/skins/plastik_dark/title/right.png

And still no arrows :(

This revision now requires changes to proceed.Dec 10 2017, 11:36 AM

Updated corrupted png files.

@alexeymin Thank you for helping me find the problem (with your arc patch tip).

It turns out that the new png files were corrupted in this patch. Most likely because I didn't know how to use the arc tool properly, and I was using git format-patch --binary HEAD~1 and pasting the generated patch as a new diff each time I was updating D7727.
It should work, but it turns out the png files were corrupted. You can see that in my yakuake github fork (e.g. https://github.com/cyberang3l/yakuake/blob/scroll-tabs/data/skins/default/tabs/left_down.png) the png files were not corrupted and looked as expected.

I simply re-exported the arrow pngs now and updated this diff with arc diff --update D7727:

Tested with the following set of commands and seems to be working now:

git clone git@github.com:KDE/yakuake.git yak
cd yak
arc patch D7727
mkdir build
cd build
cmake -DCMAKE_INSTALL_PREFIX=/usr/ ../
make
sudo make install
/usr/bin/yakuake

Can you please test for one more time yourself and see how we will proceed with this patch? If there is anything else that you believe is needed to be done, please let me know.

vtasoulas updated this revision to Diff 23727.Dec 10 2017, 3:59 PM

This comment was removed by vtasoulas.

vtasoulas updated this revision to Diff 23728.Dec 10 2017, 4:02 PM

This comment was removed by vtasoulas.

Grrrr! I messed up completely the patch with the arc tool.
Please don't test until I fix it. I will ping again. Sorry for the noise.

Added tabbar scrolling support when the selected button cannot fit in the tabbar. Changed the way that the tabbar is getting drawn in order to support a scrollbar in the future. Now the buttons in the tabbar are all getting drawn in an off-screen private pixmap, called the buttonBar, and the buttonBar is getting scrolled as needed and painted to the widget at once.
Added scroll buttons in the tabbar.
Added support for the tabbar scroll buttons in the default (Breeze) theme.
Selecting the correct tabs for double click actions

Done. Sorry again for the noise. Things should be working as expected now.

I tested with the following set of commands:

git clone git@github.com:KDE/yakuake.git yak
cd yak
arc patch D7727
mkdir build
cd build
cmake -DCMAKE_INSTALL_PREFIX=/usr/ ../
make
sudo make install
/usr/bin/yakuake

Yes, now it's OK. I see arrows. No libpng errors. And tabs, switching are working as expected.

Well, to me code looks good (except toStdString()), I don't see anything more. And at least it works (I'll try to use this build of yakuake for a few days to test).
What about other skins, btw?

app/genericfuncs.cpp
74	maybe `qMax(pixmap1.height(), pixmap2.height())` ? not a blocker though
app/skin.cpp
64	I think you can just use C++11 initializer `QStringList acceptedScrollButtonLayouts{QLatin1String("leftRight"), QLatin1String("bothLeft"), QLatin1String("bothRight")};` not sure about yakyake compiler requirements, not a blocker
app/tabbar.cpp
112	why not newer-style connect syntax? like `connect(m_selectNextTab, &QPushButton::clicked, this, &TabBar::selectNextTab);` ? To keep the same style with surrounding code?
142	I'm not sure that `toStdString()` is required here, QString has operator==() for `const char *` too. Or maybe the right part could be QLatin1String, like `m_skin->tabBarScrollButtonsLayout() == QLatin1String("bothLeft")` ?
464	Why is labmda required here? To keep x_end const? :)

Improvements as suggested by alexeymin.

app/tabbar.cpp
112	Yes, the reason I used the `connect` like this is in order to keep the same style with already existing code.
142	I changed that to comparing with the `QLatin1String("bothLeft")` code. The problem is that the operator for comparing with `const char *` is not working in a QObject as explained here: http://doc.qt.io/qt-5/qobject.html No Copy Constructor or Assignment Operator QObject has neither a copy constructor nor an assignment operator. This is by design. Actually, they are declared, but in a private section with the macro Q_DISABLE_COPY(). In fact, all Qt classes derived from QObject (direct or indirect) use this macro to declare their copy constructor and assignment operator to be private. The reasoning is found in the discussion on Identity vs Value on the Qt Object Model page.
464	Yes, exactly. I want to initialize `x_end` at the beginning of the function `paintEvent`, but mark it as `const`. I either had to use a lambda, or an initialization function. I believe a lambda is a cleaner option.

Hopefully fixing the messed up previous commit.

OK, suggestions incorporated and hopefully I will not mess up again with arc.

I believe I understood the workflow now. Sorry again for the additional noise :(

About the other skins, I can fix them once this patch gets merged.
The nice thing is that the old skins continue to work, however, no arrow buttons will be shown.
So yakuake will look like it used to, and scroll will only be working with the mouse wheel or shift+left/right.

Well, I tested it again, tried resizing yakuake window, closing random tabs, etc and everything is drawn OK and works fine. I think Eike @hein shoud say his final word.

app/tabbar.cpp
142	I don't understand why do you mention QObject, because QString is not a QObject and has nice operator=() etc... And `tabBarScrollButtonsLayout()` returns a QString, do I miss something? Anyways now it looks fine for me.

vtasoulas added inline comments.Dec 12 2017, 9:50 PM

app/tabbar.cpp
142	Stupid me :/ Sorry. When I try to compare a `QString` with a `const char ` I get the following compile error: $ make -j2 [ 4%] Automatic moc for target yakuake [ 4%] Built target yakuake_automoc Scanning dependencies of target yakuake [ 8%] Building CXX object app/CMakeFiles/yakuake.dir/tabbar.cpp.o In file included from /usr/include/x86_64-linux-gnu/qt5/QtCore/qbytearray.h:687:0, from /usr/include/x86_64-linux-gnu/qt5/QtCore/qbytearraylist.h:47, from /usr/include/x86_64-linux-gnu/qt5/QtCore/qlist.h:1093, from /usr/include/x86_64-linux-gnu/qt5/QtCore/qstringlist.h:41, from /usr/include/x86_64-linux-gnu/qt5/QtGui/qcolor.h:46, from /usr/include/x86_64-linux-gnu/qt5/QtGui/qpixmap.h:45, from /usr/include/x86_64-linux-gnu/qt5/QtGui/QPixmap:1, from /home/user/yakuake-addarrows/app/genericfuncs.h:24, from /home/user/yakuake-addarrows/app/tabbar.cpp:23: /usr/include/x86_64-linux-gnu/qt5/QtCore/qstring.h: In member function ‘void TabBar::applySkin()’: /usr/include/x86_64-linux-gnu/qt5/QtCore/qstring.h:793:5: error: ‘QString::QString(const char)’ is private QString(const char ch); ^ /home/cyber/yakuake-addarrows/app/tabbar.cpp:143:48: error: within this context if (m_skin->tabBarScrollButtonsLayout() == "bothLeft") { ^ app/CMakeFiles/yakuake.dir/build.make:206: recipe for target 'app/CMakeFiles/yakuake.dir/tabbar.cpp.o' failed make[2]: [app/CMakeFiles/yakuake.dir/tabbar.cpp.o] Error 1 CMakeFiles/Makefile2:150: recipe for target 'app/CMakeFiles/yakuake.dir/all' failed make[1]: * [app/CMakeFiles/yakuake.dir/all] Error 2 Makefile:138: recipe for target 'all' failed make: *** [all] Error 2 So after I searched around for this error `is private` I actually stumbled upon this QObject explanation and somehow I thought that since yakuake is inheriting from QWidget which is inheriting from QObject, this should be somehow related. It turns out that as the QString documentation (the correct one this time) describes here: http://doc.qt.io/qt-5/qstring.html#operator-eq-eq-1 You can disable this operator by defining QT_NO_CAST_FROM_ASCII when you compile your applications. This can be useful if you want to ensure that all user-visible strings go through QObject::tr(), for example. Then if you check the yakuake `CMakeLists.txt` file, you will find that `-DQT_NO_CAST_FROM_ASCII` is defined, which prevents the `==` operator (and other operators) to work directly on `const char *`. When I comment out the `-DQT_NO_CAST_FROM_ASCII` line from the `CMakeLists.txt` file, things work as expected. So I believe we should stick with the `QLatin1String` comparison in this case.

app/tabbar.cpp

142

Stupid me :/ Sorry.

When I try to compare a QString with a const char * I get the following compile error:

$ make -j2
[  4%] Automatic moc for target yakuake
[  4%] Built target yakuake_automoc
Scanning dependencies of target yakuake
[  8%] Building CXX object app/CMakeFiles/yakuake.dir/tabbar.cpp.o
In file included from /usr/include/x86_64-linux-gnu/qt5/QtCore/qbytearray.h:687:0,
                 from /usr/include/x86_64-linux-gnu/qt5/QtCore/qbytearraylist.h:47,
                 from /usr/include/x86_64-linux-gnu/qt5/QtCore/qlist.h:1093,
                 from /usr/include/x86_64-linux-gnu/qt5/QtCore/qstringlist.h:41,
                 from /usr/include/x86_64-linux-gnu/qt5/QtGui/qcolor.h:46,
                 from /usr/include/x86_64-linux-gnu/qt5/QtGui/qpixmap.h:45,
                 from /usr/include/x86_64-linux-gnu/qt5/QtGui/QPixmap:1,
                 from /home/user/yakuake-addarrows/app/genericfuncs.h:24,
                 from /home/user/yakuake-addarrows/app/tabbar.cpp:23:
/usr/include/x86_64-linux-gnu/qt5/QtCore/qstring.h: In member function ‘void TabBar::applySkin()’:
/usr/include/x86_64-linux-gnu/qt5/QtCore/qstring.h:793:5: error: ‘QString::QString(const char*)’ is private
     QString(const char *ch);
     ^
/home/cyber/yakuake-addarrows/app/tabbar.cpp:143:48: error: within this context
     if (m_skin->tabBarScrollButtonsLayout() == "bothLeft") {
                                                ^
app/CMakeFiles/yakuake.dir/build.make:206: recipe for target 'app/CMakeFiles/yakuake.dir/tabbar.cpp.o' failed
make[2]: *** [app/CMakeFiles/yakuake.dir/tabbar.cpp.o] Error 1
CMakeFiles/Makefile2:150: recipe for target 'app/CMakeFiles/yakuake.dir/all' failed
make[1]: *** [app/CMakeFiles/yakuake.dir/all] Error 2                                                                                                                                                                              
Makefile:138: recipe for target 'all' failed                                                                                                                                                                                       
make: *** [all] Error 2

So after I searched around for this error is private I actually stumbled upon this QObject explanation and somehow I thought that since yakuake is inheriting from QWidget which is inheriting from QObject, this should be somehow related.

It turns out that as the QString documentation (the correct one this time) describes here: http://doc.qt.io/qt-5/qstring.html#operator-eq-eq-1

You can disable this operator by defining QT_NO_CAST_FROM_ASCII when you compile your applications. This can be useful if you want to ensure that all user-visible strings go through QObject::tr(), for example.

Then if you check the yakuake CMakeLists.txt file, you will find that -DQT_NO_CAST_FROM_ASCII is defined, which prevents the == operator (and other operators) to work directly on const char *.

When I comment out the -DQT_NO_CAST_FROM_ASCII line from the CMakeLists.txt file, things work as expected.

So I believe we should stick with the QLatin1String comparison in this case.

alexeymin added inline comments.Dec 13 2017, 6:17 AM

app/tabbar.cpp
142	Ah well, with QT_NO_CAST_FROM_ASCII the only option is comparing to QLatin1String, which is probably better than QString::toUtf8() used to convert and copy to a new std::string.

elvisangelaccio removed a reviewer: KDE Applications.Jun 2 2018, 1:32 PM

This revision is now accepted and ready to land.Jun 2 2018, 1:32 PM

			Path
M			app/CMakeLists.txt (1 line)
A	M		app/genericfuncs.h (32 lines)
A	M		app/genericfuncs.cpp (81 lines)
M			app/skin.h (16 lines)
M			app/skin.cpp (29 lines)
M			app/tabbar.h (8 lines)
M			app/tabbar.cpp (221 lines)
M			data/skins/default/CMakeLists.txt (8 lines)
M			data/skins/default/tabs.skin (20 lines)
A	M	(img)	data/skins/default/tabs/left_down.png
A	M		data/skins/default/tabs/left_down.svg (85 lines)
A	M	(img)	data/skins/default/tabs/left_up.png
A	M		data/skins/default/tabs/left_up.svg (85 lines)
A	M	(img)	data/skins/default/tabs/right_down.png
A	M		data/skins/default/tabs/right_down.svg (85 lines)
A	M	(img)	data/skins/default/tabs/right_up.png
A	M		data/skins/default/tabs/right_up.svg (85 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	19292			Sep 7 2017, 8:23 PM	★	★
Diff 2	19431			Sep 11 2017, 11:28 PM	★	★
Diff 3	23724	1ae30d4	- Updated corrupted png files.	Dec 10 2017, 2:58 PM	★	★
Diff 4	23727	6a27cd5	- Selecting the correct tabs for double click actions	Dec 10 2017, 3:59 PM	★	★
Diff 5	23728	6a27cd5	Selecting the correct tabs for double click actions.	Dec 10 2017, 4:02 PM	★	★
Diff 6	23730	1ae30d4	- Added tabbar scrolling support when the selected button cannot fit in the…	Dec 10 2017, 4:25 PM	★	★
Diff 7	23775	f18b479	- Improvements as suggested by alexeymin.	Dec 11 2017, 9:01 PM	★	★
Diff 8	23779	1ae30d4	Hopefully fixing the messed up previous commit.	Dec 11 2017, 9:12 PM	★	★

Commit	Tree	Parents	Author	Summary	Date
47bf2232ba46	52faf1cf4761	66e4fd844218	Vangelis Tasoulas	Improvements as suggested by alexeymin. (Show More…)	Dec 11 2017, 8:58 PM
66e4fd844218	19cffdeb98bc	912a2e640496	Vangelis Tasoulas	Selecting the correct tabs for double click actions (Show More…)	Dec 10 2017, 3:52 PM
912a2e640496	66322a3d44b5	3d95929b9ecc	Vangelis Tasoulas	Added support for the tabbar scroll buttons in the default (Breeze) theme. (Show More…)	Sep 11 2017, 11:17 PM
3d95929b9ecc	5775b114e2da	53ba52761a11	Vangelis Tasoulas	Added scroll buttons in the tabbar. (Show More…)	Sep 11 2017, 11:06 PM
53ba52761a11	2b9339c7466c	1ae30d412d53	Vangelis Tasoulas	Added tabbar scrolling support when the selected button cannot fit in the… (Show More…)	Sep 8 2017, 12:05 PM

Bug 339139: Added tabbar scrolling support when the selected button cannot fit in the tabbar.
AcceptedPublic
Actions

Details

Diff Detail

Revision Contents
Changeset List

Diff 23779

app/CMakeLists.txt

app/genericfuncs.h

app/genericfuncs.cpp

app/skin.h

app/skin.cpp

app/tabbar.h

app/tabbar.cpp

data/skins/default/CMakeLists.txt

data/skins/default/tabs.skin

data/skins/default/tabs/left_down.png

data/skins/default/tabs/left_down.svg

data/skins/default/tabs/left_up.png

data/skins/default/tabs/left_up.svg

data/skins/default/tabs/right_down.png

data/skins/default/tabs/right_down.svg

data/skins/default/tabs/right_up.png

data/skins/default/tabs/right_up.svg

Bug 339139: Added tabbar scrolling support when the selected button cannot fit in the tabbar.AcceptedPublicActions

Details

Diff Detail

Revision ContentsChangeset List

Diff 23779

app/CMakeLists.txt

app/genericfuncs.h

app/genericfuncs.cpp

app/skin.h

app/skin.cpp

app/tabbar.h

app/tabbar.cpp

data/skins/default/CMakeLists.txt

data/skins/default/tabs.skin

data/skins/default/tabs/left_down.png

data/skins/default/tabs/left_down.svg

data/skins/default/tabs/left_up.png

data/skins/default/tabs/left_up.svg

data/skins/default/tabs/right_down.png

data/skins/default/tabs/right_down.svg

data/skins/default/tabs/right_up.png

data/skins/default/tabs/right_up.svg

Bug 339139: Added tabbar scrolling support when the selected button cannot fit in the tabbar.
AcceptedPublic
Actions

Revision Contents
Changeset List