Date:   Sun, 28 Jun 2020 14:20:41 +0300
From:   Maxim Levitsky <mlevitsk@...hat.com>
To:     Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
Cc:     linux-kernel@...r.kernel.org
Subject: Re: Search function in xconfig is partially broken after recent
 changes

On Sun, 2020-06-28 at 12:54 +0200, Mauro Carvalho Chehab wrote:
> Em Sun, 28 Jun 2020 11:37:08 +0300
> Maxim Levitsky <mlevitsk@...hat.com> escreveu:
> 
> > On Thu, 2020-06-25 at 17:05 +0200, Mauro Carvalho Chehab wrote:
> > > Em Thu, 25 Jun 2020 15:53:46 +0300
> > > Maxim Levitsky <mlevitsk@...hat.com> escreveu:
> > >   
> > > > On Thu, 2020-06-25 at 13:17 +0200, Mauro Carvalho Chehab wrote:  
> > > > > Em Thu, 25 Jun 2020 12:59:15 +0200
> > > > > Mauro Carvalho Chehab <mchehab+huawei@...nel.org> escreveu:
> > > > >     
> > > > > > Hi Maxim,
> > > > > > 
> > > > > > Em Thu, 25 Jun 2020 12:25:10 +0300
> > > > > > Maxim Levitsky <mlevitsk@...hat.com> escreveu:
> > > > > >     
> > > > > > > Hi!
> > > > > > > 
> > > > > > > I noticed that on recent kernels the search function in xconfig is partially broken.
> > > > > > > This means that when you select a found entry, it is not selected in the main window,
> > > > > > > something that I often do to find some entry near the area I would like to modify,
> > > > > > > and then use main window to navigate/explore that area.
> > > > > > > 
> > > > > > > Reverting these commits helps restore the original behavier:
> > > > > > > 
> > > > > > > b311142fcfd37b58dfec72e040ed04949eb1ac86 - kconfig: qconf: fix support for the split view mode
> > > > > > > cce1faba82645fee899ccef5b7d3050fed3a3d10 - kconfig: qconf: fix the content of the main widget
> > > > > > > 
> > > > > > > I have Qt5 5.13.2 from fedora 31 (5.13.2-1.fc31)
> > > > > > > 
> > > > > > > Could you explain what these commits are supposed to fix?
> > > > > > > I mostly use the split view mode too and it does appear to work for me with these commits reverted as well.
> > > > > > >     
> > > > > > 
> > > > > > There are three view modes for qconf:
> > > > > > 
> > > > > > 	- Single
> > > > > > 	- Split
> > > > > > 	- Full
> > > > > > 
> > > > > > those got broken when gconf was converted to use Qt5, back on Kernel 3.14.
> > > > > > Those patches restore the original behavior.    
> > > > You mean xconfig/qconf? (gconf is another program that is GTK based as far as I know).  
> > > 
> > > Yeah, I meant the Qt one (qconfig).
> > >   
> > > > Could you expalin though what was broken? What exactly didn't work?  
> > > 
> > > Try to switch between the several modes and switch back. There used to
> > > have several broken things there, because the Qt5 port was incomplete.
> > > 
> > > One of the things that got fixed on the Qt5 fixup series is the helper
> > > window at the bottom. It should now have the same behavior as with the
> > > old Qt3/Qt4 version.
> > > 
> > > Basically, on split mode, it should have 3 screen areas:
> > > 
> > > 	+------------+-------+
> > > 	|            |       |
> > > 	| Config     |  menu |
> > > 	|            |       |
> > > 	+------------+-------+
> > > 	|                    |
> > > 	| Config description +
> > > 	|                    |
> > > 	+--------------------+
> > > 
> > > The contents of the config description should follow up any changes at 
> > > the "menu" part of the split mode, when an item is selected from "menu",
> > > or follow what's selected at "config", when the active window is "config".  
> > 
> > Dunno. with the 2 b311142fcfd37b58dfec72e040ed04949eb1ac86 and cce1faba82645fee899ccef5b7d3050fed3a3d10,
> > in split view, I wasn't able to make the 'config description' show anything wrong,
> > in regard to currently selected item in 'config' and in 'menu'
> 
> Well, the problem was related to how the code calls those 3 areas
> internally: configView, menuView and configInfoView. 
> 
> When it is outside the split view, it should hide the
> menuView, in order to show just the configView and the configInfoView.
> 
> There were lots of weird stuff there. I suspect that, after the
> half-done Qt5 conversion (that handled badly the menuView hiding
> logic), some hacks were added, assuming the wrong window hiding 
> logic. When I fixed it, other things stopped working. So, additional
> fixup patches were needed.
> 
> > At that point this is mostly an academic interset for me since,
> > the patch that you sent fixes search. Thank you very much!
> 
> Anytime!
> 
> > BTW, I re-discovered another bug (I have seen it already but it didn't bother me that much),
> > while trying to break the version with these commits reverted (but it happens 
> > with them not reverted as well):
> > 
> > When I enable 'show debug info' to understand why I can't enable/disable some config
> > option, the hyperlinks in the config description don't work - they make the config
> > window to be empty.
> 
> It sounds that the creation of the search list for the QTextBrowser 
> instantiated class (e. g. configInfoView) is not fine.
> 
> It sounds that it was supposed to call either setInfo() or
> setMenuLink() when a debug info hyperlink is clicked:
> 
> 	info = new ConfigInfoView(split, name);
> 	connect(list->list, SIGNAL(menuChanged(struct menu *)),
> 		info, SLOT(setInfo(struct menu *)));
> 
> But this is not happening. Perhaps this also broke with the Qt5
> conversion?
> 
> I suspect it should, instead, use a different signal to handle it.
> 
> See, with the enclosed patch, clicking on a link will now show:
> 
> 	Clicked on URL QUrl("s0x21c3f10")
> 	QTextBrowser: No document for s0x21c3f10
> 
> Which helps to explain what's happening here.
> 
> See, when debug is turned on, it will create hyperlinks like:
> 
> 	head += QString().sprintf("<a href=\"s%p\">", sym);
> 
> It seems that the code needs something like:
> 
> 	connect (helpText, SIGNAL (anchorClicked (const QUrl &)),
> 			 helpText, SLOT (clicked (const QUrl &)) );
> 
> and a handler for this signal that would translate "s%p"
> back into sym, using such value to update the menus.
> 
> Do you know if this used to work after Kernel 3.14?

I don't know yet, but I can test it. 

I didn't do much kernel developement for lot of time, so I only vaguely
remember that once upon a time it did work. I don't use this feature much,
so it might as well be broken back when conversion to Qt5 happened.
Also worth noting that I probably used Qt4 untill recently when I updated
to fedora 31, which looks like dropped Qt4 developement packages.

I used to know a thing or two about Qt, long long ago, so on next weekend or so,
I can also take a look at this.

Best regards,
	Maxim Levitsky

> 
> Thanks,
> Mauro
> 
> diff --git a/scripts/kconfig/qconf.cc b/scripts/kconfig/qconf.cc
> index b8f577c6e8aa..4d9bf9330c73 100644
> --- a/scripts/kconfig/qconf.cc
> +++ b/scripts/kconfig/qconf.cc
> @@ -4,27 +4,19 @@
>   * Copyright (C) 2015 Boris Barbulovski <bbarbulovski@...il.com>
>   */
>  
> -#include <qglobal.h>
> -
> -#include <QMainWindow>
> -#include <QList>
> -#include <qtextbrowser.h>
>  #include <QAction>
> +#include <QApplication>
> +#include <QCloseEvent>
> +#include <QDebug>
> +#include <QDesktopWidget>
>  #include <QFileDialog>
> +#include <QLabel>
> +#include <QLayout>
> +#include <QList>
>  #include <QMenu>
> -
> -#include <qapplication.h>
> -#include <qdesktopwidget.h>
> -#include <qtoolbar.h>
> -#include <qlayout.h>
> -#include <qsplitter.h>
> -#include <qlineedit.h>
> -#include <qlabel.h>
> -#include <qpushbutton.h>
> -#include <qmenubar.h>
> -#include <qmessagebox.h>
> -#include <qregexp.h>
> -#include <qevent.h>
> +#include <QMenuBar>
> +#include <QMessageBox>
> +#include <QToolBar>
>  
>  #include <stdlib.h>
>  
> @@ -400,6 +392,8 @@ void ConfigList::updateSelection(void)
>  	struct menu *menu;
>  	enum prop_type type;
>  
> +qInfo() << __FUNCTION__;
> +
>  	if (mode == symbolMode)
>  		setHeaderLabels(QStringList() << "Item" << "Name" << "N" << "M" << "Y" << "Value");
>  	else
> @@ -536,6 +530,8 @@ void ConfigList::setRootMenu(struct menu *menu)
>  {
>  	enum prop_type type;
>  
> +
> +qInfo() << __FUNCTION__ << "menu:" << menu;
>  	if (rootEntry == menu)
>  		return;
>  	type = menu && menu->prompt ? menu->prompt->type : P_UNKNOWN;
> @@ -1020,6 +1016,7 @@ void ConfigView::updateListAll(void)
>  ConfigInfoView::ConfigInfoView(QWidget* parent, const char *name)
>  	: Parent(parent), sym(0), _menu(0)
>  {
> +qInfo() << __FUNCTION__;
>  	setObjectName(name);
>  
>  
> @@ -1033,6 +1030,7 @@ ConfigInfoView::ConfigInfoView(QWidget* parent, const char *name)
>  
>  void ConfigInfoView::saveSettings(void)
>  {
> +qInfo() << __FUNCTION__;
>  	if (!objectName().isEmpty()) {
>  		configSettings->beginGroup(objectName());
>  		configSettings->setValue("/showDebug", showDebug());
> @@ -1042,6 +1040,7 @@ void ConfigInfoView::saveSettings(void)
>  
>  void ConfigInfoView::setShowDebug(bool b)
>  {
> +qInfo() << __FUNCTION__;
>  	if (_showDebug != b) {
>  		_showDebug = b;
>  		if (_menu)
> @@ -1054,6 +1053,8 @@ void ConfigInfoView::setShowDebug(bool b)
>  
>  void ConfigInfoView::setInfo(struct menu *m)
>  {
> +qInfo() << __FUNCTION__ << "menu:" << m;
> +
>  	if (_menu == m)
>  		return;
>  	_menu = m;
> @@ -1068,6 +1069,8 @@ void ConfigInfoView::symbolInfo(void)
>  {
>  	QString str;
>  
> +qInfo() << __FUNCTION__;
> +
>  	str += "<big>Symbol: <b>";
>  	str += print_filter(sym->name);
>  	str += "</b></big><br><br>value: ";
> @@ -1085,6 +1088,8 @@ void ConfigInfoView::menuInfo(void)
>  	struct symbol* sym;
>  	QString head, debug, help;
>  
> +qInfo() << __FUNCTION__;
> +
>  	sym = _menu->sym;
>  	if (sym) {
>  		if (_menu->prompt) {
> @@ -1140,6 +1145,7 @@ QString ConfigInfoView::debug_info(struct symbol *sym)
>  {
>  	QString debug;
>  
> +qInfo() << __FUNCTION__;
>  	debug += "type: ";
>  	debug += print_filter(sym_type_name(sym->type));
>  	if (sym_is_choice(sym))
> @@ -1191,6 +1197,7 @@ QString ConfigInfoView::debug_info(struct symbol *sym)
>  
>  QString ConfigInfoView::print_filter(const QString &str)
>  {
> +qInfo() << __FUNCTION__;
>  	QRegExp re("[<>&\"\\n]");
>  	QString res = str;
>  	for (int i = 0; (i = res.indexOf(re, i)) >= 0;) {
> @@ -1225,6 +1232,7 @@ void ConfigInfoView::expr_print_help(void *data, struct symbol *sym, const char
>  	QString* text = reinterpret_cast<QString*>(data);
>  	QString str2 = print_filter(str);
>  
> +qInfo() << __FUNCTION__;
>  	if (sym && sym->name && !(sym->flags & SYMBOL_CONST)) {
>  		*text += QString().sprintf("<a href=\"s%p\">", sym);
>  		*text += str2;
> @@ -1233,11 +1241,17 @@ void ConfigInfoView::expr_print_help(void *data, struct symbol *sym, const char
>  		*text += str2;
>  }
>  
> +void ConfigInfoView::clicked(const QUrl &url)
> +{
> +	qInfo() << "Clicked on URL" << url;
> +}
> +
>  QMenu* ConfigInfoView::createStandardContextMenu(const QPoint & pos)
>  {
>  	QMenu* popup = Parent::createStandardContextMenu(pos);
>  	QAction* action = new QAction("Show Debug Info", popup);
>  
> +qInfo() << __FUNCTION__;
>  	action->setCheckable(true);
>  	connect(action, SIGNAL(toggled(bool)), SLOT(setShowDebug(bool)));
>  	connect(this, SIGNAL(showDebugChanged(bool)), action, SLOT(setOn(bool)));
> @@ -1249,6 +1263,7 @@ QMenu* ConfigInfoView::createStandardContextMenu(const QPoint & pos)
>  
>  void ConfigInfoView::contextMenuEvent(QContextMenuEvent *e)
>  {
> +qInfo() << __FUNCTION__;
>  	Parent::contextMenuEvent(e);
>  }
>  
> @@ -1258,6 +1273,8 @@ ConfigSearchWindow::ConfigSearchWindow(ConfigMainWindow* parent, const char *nam
>  	setObjectName(name);
>  	setWindowTitle("Search Config");
>  
> +qInfo() << __FUNCTION__ << "name:" << name;
> +
>  	QVBoxLayout* layout1 = new QVBoxLayout(this);
>  	layout1->setContentsMargins(11, 11, 11, 11);
>  	layout1->setSpacing(6);
> @@ -1506,6 +1523,9 @@ ConfigMainWindow::ConfigMainWindow(void)
>  	helpMenu->addAction(showIntroAction);
>  	helpMenu->addAction(showAboutAction);
>  
> +	connect (helpText, SIGNAL (anchorClicked (const QUrl &)),
> +		 helpText, SLOT (clicked (const QUrl &)) );
> +
>  	connect(configList, SIGNAL(menuChanged(struct menu *)),
>  		helpText, SLOT(setInfo(struct menu *)));
>  	connect(configList, SIGNAL(menuSelected(struct menu *)),
> @@ -1603,6 +1623,7 @@ void ConfigMainWindow::saveConfigAs(void)
>  
>  void ConfigMainWindow::searchConfig(void)
>  {
> +qInfo() << __FUNCTION__;
>  	if (!searchWindow)
>  		searchWindow = new ConfigSearchWindow(this, "search");
>  	searchWindow->show();
> @@ -1610,6 +1631,11 @@ void ConfigMainWindow::searchConfig(void)
>  
>  void ConfigMainWindow::changeItens(struct menu *menu)
>  {
> +qInfo() << __FUNCTION__ << "Changing to menu" << menu;
> +
> +	if (menu->flags & MENU_ROOT)
> +		qInfo() << "Wrong type when changing item";
> +
>  	configList->setRootMenu(menu);
>  
>  	if (configList->rootEntry->parent == &rootmenu)
> @@ -1620,6 +1646,11 @@ void ConfigMainWindow::changeItens(struct menu *menu)
>  
>  void ConfigMainWindow::changeMenu(struct menu *menu)
>  {
> +qInfo() << __FUNCTION__ << "Changing to menu" << menu;
> +
> +	if (!(menu->flags & MENU_ROOT))
> +		qInfo() << "Wrong type when changing menu";
> +
>  	menuList->setRootMenu(menu);
>  
>  	if (menuList->rootEntry->parent == &rootmenu)
> @@ -1633,6 +1664,7 @@ void ConfigMainWindow::setMenuLink(struct menu *menu)
>  	struct menu *parent;
>  	ConfigList* list = NULL;
>  	ConfigItem* item;
> +qInfo() << __FUNCTION__ << "Changing to menu" << menu;
>  
>  	if (configList->menuSkip(menu))
>  		return;
> @@ -1681,6 +1713,7 @@ void ConfigMainWindow::setMenuLink(struct menu *menu)
>  
>  void ConfigMainWindow::listFocusChanged(void)
>  {
> +qInfo() << __FUNCTION__;
>  	if (menuList->mode == menuMode)
>  		configList->clearSelection();
>  }
> @@ -1689,6 +1722,7 @@ void ConfigMainWindow::goBack(void)
>  {
>  	ConfigItem* item, *oldSelection;
>  
> +qInfo() << __FUNCTION__;
>  	configList->setParentMenu();
>  	if (configList->rootEntry == &rootmenu)
>  		backAction->setEnabled(false);
> diff --git a/scripts/kconfig/qconf.h b/scripts/kconfig/qconf.h
> index c879d79ce817..a193137f2314 100644
> --- a/scripts/kconfig/qconf.h
> +++ b/scripts/kconfig/qconf.h
> @@ -3,17 +3,17 @@
>   * Copyright (C) 2002 Roman Zippel <zippel@...ux-m68k.org>
>   */
>  
> -#include <QTextBrowser>
> -#include <QTreeWidget>
> -#include <QMainWindow>
> +#include <QCheckBox>
> +#include <QDialog>
>  #include <QHeaderView>
> -#include <qsettings.h>
> +#include <QLineEdit>
> +#include <QMainWindow>
>  #include <QPushButton>
>  #include <QSettings>
> -#include <QLineEdit>
>  #include <QSplitter>
> -#include <QCheckBox>
> -#include <QDialog>
> +#include <QTextBrowser>
> +#include <QTreeWidget>
> +
>  #include "expr.h"
>  
>  class ConfigView;
> @@ -250,6 +250,7 @@ public slots:
>  	void setInfo(struct menu *menu);
>  	void saveSettings(void);
>  	void setShowDebug(bool);
> +	void clicked (const QUrl &url);
>  
>  signals:
>  	void showDebugChanged(bool);
> 
> 
> 

Powered by blists - more mailing lists