----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120403/#review67559 -----------------------------------------------------------
kdeui/widgets/kmenu.cpp <https://git.reviewboard.kde.org/r/120403/#comment47089> try to avoid negated bools unless for performance or readability reasons (ie. you'd need "!flag" a lot otherwise) kdeui/widgets/kmenu.cpp <https://git.reviewboard.kde.org/r/120403/#comment47091> parsing associatedWidgets (eg. a QMenu or a QMenuBar or a QToolButton) is a good idea, but what you want to do is to search yourself through pot. QMenu's until one ::menuAction() is associated to a QMenuBar (ie. "this menu can be reached by navigating through a menubar and its menus) kdeui/widgets/kmenu.cpp <https://git.reviewboard.kde.org/r/120403/#comment47092> since KMenu inherits QMenu. "qobject_cast<KMenu*>(w)" implies "qobject_cast<QMenu*>", iow. testing for "qobject_cast<QMenu*>(w)" would be sufficient kdeui/widgets/kmenu.cpp <https://git.reviewboard.kde.org/r/120403/#comment47093> As mentioned before, the parentship is pretty meaningless here. A popup *might* be parented by a window that has it in its menubar, but doesn't have to at all. kdeui/widgets/kmenu.cpp <https://git.reviewboard.kde.org/r/120403/#comment47094> If I understand the Qt doc correctly, there're basically two kinds of menubars: 1) per window 2) shared however, they all will end up as global menu? Even if not: how many menubars are not mapped to the global one (the I could think of Qt Designer forms) and are those false positives actually worth the false negatives (ie. you miss special casing a menu)? kdeui/widgets/kmenu.cpp <https://git.reviewboard.kde.org/r/120403/#comment47095> I'd try to move common tasks (setting icon and text etc.) out of the branch, what will also resolve... kdeui/widgets/kmenu.cpp <https://git.reviewboard.kde.org/r/120403/#comment47096> .... mehhh: branched exits. kdeui/widgets/kmenu.cpp <https://git.reviewboard.kde.org/r/120403/#comment47097> you might #ifdef Q_OS_MAC this branch as well. - Thomas Lübking On Sept. 28, 2014, 12:46 nachm., René J.V. Bertin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/120403/ > ----------------------------------------------------------- > > (Updated Sept. 28, 2014, 12:46 nachm.) > > > Review request for KDE Software on Mac OS X, kdelibs and Qt KDE. > > > Repository: kdelibs > > > Description > ------- > > This is a spin-off of RR https://git.reviewboard.kde.org/r/120355/ > > As described in that RR, OS X cannot render menu items that were created > using `KMenu::addTitle`. Without a Qt patch, that function will even provoke > a crash. With a patch, the items are rendered barely legibly once, and then > render as empty space in subsequent menu openings. > The main object of that RR is to present and discuss a workaround at the > client level, emulating `KMenu::addTitle`. In this RR, I present a draft > adaptation of that function itself. > > The main goal as I see it is to modify the function just enough so that it > changes its behaviour for items that will or can be rendered in the Mac's > global menubar, using non-Qt code. Pop-up menus that are not attached to that > structure are rendered through Qt and can show all the style formatting they > can under Linux as long as it's not tied to X11 directly. > It's probably impossible to cater to all possible use cases, so I'd propose > to focus on the situations we can detect from inside `KMenu::addTitle`. That > is, *if* we want to ease the client's burden of obtaining a sensible menu > item *and* we want to maintain support for the intended/expected style in the > menus that can actually support them. (KDevelop's context menu its Project > view is a prime example.) > The other goal (secondary for KDE/Mac for the time being) is to come up with > a patch proposal for Qt5's `QMenu::addSection`, because its current use of > texted separators makes it equivalent to `QMenu::addSeparator` on OS X. (= > you get a separator instead of an item showing the title text.) > > I have not found a way to detect reliably that a menu is attached to the > global menubar. There are functions that are supposed to allow this > (KMenu::isTopLevelMenu, QMenu::macMenu) but they don't work as expected. So > what I propose here is to emulate the styled menu title when adding to a > KMenu that is somehow associated to a KMenu belonging to a KMainWindow that > has a menubar. This can probably lead to false hits, and I have already > learned that it doesn't catch all the intended cases either (e.g. > MessageList->Sorting menu under KMail's View menu is apparently not yet > attached when addTitle is called). So I'm following Thomas's suggestion to > publish the draft for feedback. > > In case anyone wonders about the emulation code (when notMacMenuBar==false): > I'm open for suggestions but this is the only approach I've found to create > an entry that stands out. It's not impossible to to obtain the font OS X uses > for menubar menu items (Lucida Grande 14; requires 2 extra functions), but > changing font attributes (bold, underline, overline etc) has no effect. (The > bold font version file is available, so it might be possible to get the bold > font rendered by specifying it as a full font spec and not as an attribute > but I wouldn't know how to achieve this.) > > > Diffs > ----- > > kdeui/widgets/kmenu.cpp 7dab149 > > Diff: https://git.reviewboard.kde.org/r/120403/diff/ > > > Testing > ------- > > On OS X 10.6.8 with kdelibs git/kde4.14 . Currently the draft does nothing on > other OSes, and the actual "emulation" code (when notMacMenuBar==false) can > go conditional if there are no other platforms where a similar approach could > be desired. > > > Thanks, > > René J.V. Bertin > >
