From kde-core-devel Thu Oct 29 06:42:28 2009 From: "Dawit A." Date: Thu, 29 Oct 2009 06:42:28 +0000 To: kde-core-devel Subject: Re: kdewebkit moved to kdereview Message-Id: <200910290242.28715.adawit () kde ! org> X-MARC-Message: https://marc.info/?l=kde-core-devel&m=125679862317163 On Wednesday 28 October 2009 18:43:02 Benjamin Meyer wrote: > Thanks for fixing up the other stuff. > > >> openUrlInNewTab docs are incorrect. It is also emited if you ctrl > >> click on a link. > >> openUrlInNewWindow seems to be missing for when the user would click > >> on a link with those keyboard modifiers. > >> What about the about when the user wants to open the link in a new > >> tab and have it be selected? > > > > Fixed the documentation and renamed the signal to 'openUrlInNewWindow'. > > > > Whether you want to handle that signal by opening a new window or a > > new tab or how that tab should be positioned afterwards is an application > > specific issue. This class have no means of even knowing that your > > application uses tabs. > > Hmmm I guess openUrlInNewWindows is just as bad for the same reason. > How about calling it something like middleClickedOnLink() ? hmm... Sounds good to me but what the CTRL+click causing this very signal to be emitted ? > >> The wheel event code does not do the same thing in qt 4.5 and 4.6. > >> Should this wheel behavior actually go upstream into qtwebkit? > > > > Ahhh... There is only a "#if QT_VERSION < 0x040500" in that code to > > make it work with QtWebKit from Qt 4.4 so I do not understand the 4.6 > > reference in your comment... > > Sorry 4.4 and 4.5 Seeing as how KDE requires 4.5 I guess we can just > remove that. Yes, that is bagage carried over from the initial work. In fact that function might need some more work since attempt to zoom in/out of pages quickly produces noticable lag. > > As far as that function is concerned, it is only used to do page > > zooming. I am not entirely sure that this would be accepted upstream since > > one can implement this functionality by simply installing an event filter > > on the QWebPage. It is added here following the theme of a convenience > > function... > > I think this is something that could go upstream. It is worth asking > on #qtwebkit at least or on the qtwebkit mailinglist. No objection to it going upstream... > >> There seems to be missing convenience functions to get the kde > >> network access manager and plugin factory > > > > Ahem... I do not follow... There is already a > > QWebPage::networkAccessManager() and QWebPage::pluginFactory(). > > Yes, but those return a QNetworkAccessManager pointer. It would be > nice to have a function that returned the kde network object or 0 if > it is not set (i.e. do the cast for me) Well the problem with that is I have to use the same accessor name, networkAccessManager(), which will effectively hide the member function in the parent class. Hence, you will not be able to get back a reference to QNetworkAccessManager if do not use a KIO::AccessManager on this class. That behavior would be different from the version in the parent class. Or are you asking I use a different name perhaps something like "netAccessManager()", but that would be even more confusing... > > KWebPluginFactory is a pure re-implementation of existing virtual > > member functions and does not provide any additional methods. For the > > network access manager, if you invoked > > QWebPage::setNetworkAccessManager(...) with an instance to a re- > > implementation, then you already have access to that instance > > or you should simply use qobject_cast to down cast what is returned by > > QWebPage::networkAccessManager.... So I fail to understand what you > > mean here... > > > >> KWebPage sets a ton of icons on the actions when the KWebPage is > >> created. This slows down the startup time and really upstream should > >> get a patch to use QIcon::fromTheme if it doesn't already. Same goes > >> for the shortcuts. > > > > QIcon::fromTheme ? New function in Qt 4.6 ? I do not see that > > function in the QIcon class documentation. I think there is no objection > > to do that if it is possible, but you have to elaborate on what you > > mean... > > Yah it is a new function in 4.6. It lets you request freedesktop > icons and it is obtained from the theme. For those actions that have > freedesktop icon names we should fix upstream to help reduce the > startup load. Already did a #if for this and I do not mind this stuff going upstream for the ones that can be done there, specially if it reduces unecessary load time. How about the shortcut keys ? Are they already taken care of by QKeySequence ? > >> KWebPage::downloadRequest > >> - Shouldn't this call to kget over dcop? > > > > Yes and that is on the TODO list. This code was lifted out of > > khtml ; so it probably needs to be fixed there as well... > > Thinking about this some more perhaps this means that any application > that uses QWebPage a webpage can cause file dialog popups to appear. > Perhaps having a property to handle downloadRequests via kio should be > added and have it off by default. I do not really follow what you are saying here. What do you mean by "any application that uses QWebPage a webpage can cause file dialog popups to appear" ?