Bug 132678 - JJ: Right mouse click - search at google - ampersand issue.
Summary: JJ: Right mouse click - search at google - ampersand issue.
Status: RESOLVED FIXED
Alias: None
Product: kdelibs
Classification: Unmaintained
Component: general (show other bugs)
Version: unspecified
Platform: Compiled Sources Linux
: NOR normal
Target Milestone: ---
Assignee: Stephan Kulow
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-08-20 10:26 UTC by Nick Warne
Modified: 2008-03-04 18:57 UTC (History)
1 user (show)

See Also:
Latest Commit:
Version Fixed In:
Sentry Crash Report:


Attachments
132678.patch (869 bytes, application/octet-stream)
2006-08-27 17:47 UTC, William Entriken
Details
Patch against KDE3.5.4 khtml_ext.cpp (1006 bytes, patch)
2006-08-28 10:36 UTC, Nick Warne
Details
Dirty hack to fix the percent issues (873 bytes, patch)
2006-09-09 13:24 UTC, Nick Warne
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Warne 2006-08-20 10:26:59 UTC
Version:            (using KDE KDE 3.5.4)
Installed from:    Compiled From Sources
Compiler:          GCC 3.4.6 -march=athlon-tbird
OS:                Linux

When highlighting text, right mouse click -> search at google, if the text selected contains and ampersand, the text passed (parsed) for input is cut off at the ampersand;  Select the text below, right mouse click -> search at google:

KDE & Konqueror

KDE&Konqueror

Nick
Comment 1 Dirk Stoecker 2006-08-21 13:51:56 UTC
The & is not converted to %26, but copied as is.

This works for other searching places in konqueror, so it seems related to the rightclick search only.
Comment 2 Dirk Stoecker 2006-08-21 14:34:25 UTC
The error probably is in kdelibs/khtml/khtml_ext.cpp:

KHTMLPopupGUIClient::KHTMLPopupGUIClient (near line 400).

a) The search text cannot be taken into the menu as is.
   1) Selecting & in text results in interpretation as hotkey.
   2) Selecting e.g. the text %26 above results in wrong text building, as
      %2 is interpreted as printf style marker
b) Finally URL encoding seems to be missing.

P.S. Sorry, but I'm not willing to debug kdelibs ATM.
Comment 3 Nick Warne 2006-08-21 19:11:27 UTC
OK, thanks for looking at this.

I got it so the 'menu' entry reads correct, but the stringe is still passed wrongly:

kdelibs/khtml/khtml_ext.cpp around line 444

      // search text
      QString selectedText = khtml->selectedText();
      +selectedText.replace("&", "&&");
      if ( selectedText.length()>18 ) {
        selectedText.truncate(15);
        selectedText+="...";
      }


But it's no big deal anyway... be just nice to fix next time around.

Nick
Comment 4 Dirk Stoecker 2006-08-22 12:17:19 UTC
Marked as Junior Job. I think this should be pretty easy to fix.
Comment 5 William Entriken 2006-08-27 17:47:39 UTC
---------- Forwarded message ----------
From: Will Entriken <kde.org@phor.net>
Date: Aug 27, 2006 11:46 AM
Subject: [patch] bug 132678
To: 132678@bugzilla.kde.org, kde-devel <kde-devel@kde.org>


I think I fixed this bug. It seems like there is a mix of escaped and
unescaped query strings, this fixes that.

I would commit this patch myself, but it could takes days for my
working copy to compile. Updating and compiling really sucks.


Created an attachment (id=17521)
132678.patch
Comment 6 Nick Warne 2006-08-27 18:17:03 UTC
I can't this to build:

khtml_ext.cpp: In member function `void KHTMLPartBrowserExtension::searchProvider()':
khtml_ext.cpp:276: error: incomplete type `QUrl' used in nested name specifier
make[1]: *** [libkhtml_la.all_cpp.lo] Error 1
make[1]: Leaving directory `/home/hdb/kde_build/konstruct/kde/kdelibs/work/kdelibs-3.5.4/khtml'

Nick ?
Comment 7 William Entriken 2006-08-27 18:50:07 UTC
SVN commit 577831 by entriken:

CCBUG: 132678

It worksforme, but this patch may fix Nick's problem at:
http://bugs.kde.org/show_bug.cgi?id=132678#c6



 M  +1 -0      khtml_ext.cpp  


--- trunk/KDE/kdelibs/khtml/khtml_ext.cpp #577830:577831
@@ -38,6 +38,7 @@
 #include <qclipboard.h>
 #include <qfileinfo.h>
 #include <qmenu.h>
+#include <qurl.h>
 #include <qmetaobject.h>
 
 #include <kdebug.h>
Comment 8 Nick Warne 2006-08-27 18:57:57 UTC
I don't even have the same headers included in khtml_ext.cpp (3.5.4):

#include "html/html_imageimpl.h"
#include "misc/loader.h"
#include "dom/html_form.h"
#include "dom/html_image.h"
#include <qclipboard.h>
#include <qfileinfo.h>
#include <qpopupmenu.h> ????????????
#include <qmetaobject.h>
#include <private/qucomextra_p.h>
#include <qdragobject.h>

#include <kdebug.h>
#include <klocale.h>
#include <kfiledialog.h>
Comment 9 Nick Warne 2006-08-27 19:14:35 UTC
OK, if I do add in the additional qurl.h header, I get a different build issue:

In file included from libkhtml_la.all_cpp.cpp:7:
khtml_ext.cpp: In member function `void KHTMLPartBrowserExtension::searchProvider()':
khtml_ext.cpp:277: error: `toPercentEncoding' is not a member of `QUrl'
make[1]: *** [libkhtml_la.all_cpp.lo] Error 1
make[1]: Leaving directory `/home/hdb/kde_build/konstruct/kde/kdelibs/work/kdelibs-3.5.4/khtml'


Nick
Comment 10 Nick Warne 2006-08-27 19:56:43 UTC
BUGGER ME!

I fixed this patch.  After reading QT docs, and reading build errors:

 #include <qclipboard.h> 
  #include <qfileinfo.h> 
  #include <qmenu.h> 
 +#include <qurl.h> 
  #include <qmetaobject.h> 

    if( !KURIFilter::self()->filterURI(data, list) )
    {
        KDesktopFile file("searchproviders/google.desktop", true, "services");
+        QString encodedSearchTerm = m_part->selectedText();
+        QUrl::encode(encodedSearchTerm);
        data.setData(file.readEntry("Query").replace("\\{@}", encodedSearchTerm));

      // search text 
       QString selectedText = khtml->selectedText(); 
       +selectedText.replace("&", "&&"); 
       if ( selectedText.length()>18 ) { 
         selectedText.truncate(15); 
         selectedText+="..."; 
       } 
 
I apologise for not submitting a proper patch...

Thanks, Will !

Nick
Comment 11 Nick Warne 2006-08-28 10:36:38 UTC
Created attachment 17524 [details]
Patch against KDE3.5.4 khtml_ext.cpp

This works fine... patch attached.
Comment 12 Dirk Stoecker 2006-08-31 08:32:02 UTC
SVN commit 579136 by stoecker:

CCBUG: 132678
Backport of Google search encoding fix for KDE 3.5 by Nick Warne.


 M  +5 -1      khtml_ext.cpp  


--- branches/KDE/3.5/kdelibs/khtml/khtml_ext.cpp #579135:579136
@@ -38,6 +38,7 @@
 #include <qclipboard.h>
 #include <qfileinfo.h>
 #include <qpopupmenu.h>
+#include <qurl.h>
 #include <qmetaobject.h>
 #include <private/qucomextra_p.h>
 #include <qdragobject.h>
@@ -273,7 +274,9 @@
     if( !KURIFilter::self()->filterURI(data, list) )
     {
         KDesktopFile file("searchproviders/google.desktop", true, "services");
-        data.setData(file.readEntry("Query").replace("\\{@}", m_part->selectedText()));
+	QString encodedSearchTerm = m_part->selectedText();
+	QUrl::encode(encodedSearchTerm);
+	data.setData(file.readEntry("Query").replace("\\{@}", encodedSearchTerm));
     }
 
     KParts::URLArgs args;
@@ -441,6 +444,7 @@
 
       // search text
       QString selectedText = khtml->selectedText();
+      selectedText.replace("&", "&&");
       if ( selectedText.length()>18 ) {
         selectedText.truncate(15);
         selectedText+="...";
Comment 13 Dirk Stoecker 2006-09-08 18:32:03 UTC
The reported bug is fixed, but selecting "test%2" still results in an error based  on the i18n functionality. Needs to be fixed as well. May also be a QT bug.
Comment 14 Nick Warne 2006-09-09 10:56:35 UTC
Ummm, yes - %1 (or %10, %11 etc.) breaks this also.  I think the offending line is 480:


     new KAction( i18n( "Search '%1' at %2" ).arg( selectedText ).arg( name ), icon, 0, d->m_khtml->browserExtension(),
                     SLOT( searchProvider() ), actionCollection(), "searchProvider" );


Starting to get deep...

Nick
Comment 15 Nick Warne 2006-09-09 13:24:38 UTC
Created attachment 17681 [details]
Dirty hack to fix the percent issues

.
Real dirty hack to fix this percent issue - I think to fix this properly, a lot
of diving into the code needs to done.	I can't see how both %1 and %2 get
'parsed' as argv in new Kaction if present in SelectedText?

-      new KAction( i18n( "Search '%1' at %2" ).arg( selectedText ).arg( name
), icon, 0, d->m_khtml->browserExtension(),
+      // Dirty hack to stop %1 and %2 being being somehow parsed as argv in
new Kaction when
+      // appearing in SelectedText - see bug 132678
+      QString selectedTextandName = "'" + selectedText + "' at " + name;
+      //new KAction( i18n( "Search '%1' at %2" ).arg( selectedText ).arg( name
), icon, 0, d->m_khtml->browserExtension(),
+      new KAction( i18n( "Search %1" ).arg( selectedTextandName ), icon, 0,
d->m_khtml->browserExtension(),
		      SLOT( searchProvider() ), actionCollection(),
"searchProvider" );

Nick
Comment 16 Nick Warne 2006-09-09 15:21:27 UTC
Actually, this is a very deep issue.

%26 %27 %28

Will appear correct now in right mouse click menu, but actual data sent to google (etc.) is the encoded characters [ & ' (  ].

So, with the two patches I supplied, it is better and partially fixed, but this underlying issue runs deep.

Nick
Comment 17 Dirk Stoecker 2006-09-10 22:42:44 UTC
Sorry Nick, this does not work. The types %1 and %2 are used, because other languages may require other orders of the arguments.

The problem here is that for i18n("Hello %1 %2").arg(x1).arg(x2) the parsing should not reparse the included text (x1) when scanning for %2.

Actually it is a QT bug and I will report it there and close this one.

#include <QApplication>
#include <QPushButton>
int main(int argc, char *argv[])
{
  QApplication app(argc, argv);
  QPushButton hello(QString("1:%1 2:%2 3:%3").arg("%1").arg("%2").arg("test3"));
  hello.resize(200, 50);
  hello.show();
  return app.exec();
}

Resolving as fixed, as the main bug report has been fulfilled.
Comment 18 Dirk Stoecker 2006-09-13 21:54:05 UTC
SVN commit 583923 by stoecker:

Fixed text error for certain selected texts.

CCBUG: 132678


 M  +2 -1      khtml_ext.cpp  


--- branches/KDE/3.5/kdelibs/khtml/khtml_ext.cpp #583922:583923
@@ -476,7 +476,8 @@
         name = "Google";
       }
 
-      new KAction( i18n( "Search for '%1' with %2" ).arg( selectedText ).arg( name ), icon, 0, d->m_khtml->browserExtension(),
+      // using .arg(foo, bar) instead of .arg(foo).arg(bar), as foo can contain %x
+      new KAction( i18n( "Search for '%1' with %2" ).arg( selectedText, name ), icon, 0, d->m_khtml->browserExtension(),
                      SLOT( searchProvider() ), actionCollection(), "searchProvider" );
 
       // favorite search providers
Comment 19 Nick Warne 2006-09-13 22:22:51 UTC
Hi Dirk,

Heh... so simple a fix.

Thanks, all appears to work as it should here!

Good job!

Nick