> On June 25, 2013, 6:46 a.m., Kevin Ottens wrote: > > kio/kfile/kurlrequester.cpp, line 483 > > <http://git.reviewboard.kde.org/r/111184/diff/2/?file=165624#file165624line483> > > > > Why don't you use KUrlMimeData like the original code in KLineEdit? > > It'll be more robust AFAICT. > > Albert Vaca Cintora wrote: > I'm using hasUrls() instead because KUrlMimeData returns a list of urls, > which is more expensive than what Qt does (a string comparison). > > Also, if our way of checking URLs is better than the way Qt uses, we > should contribute it to Qt instead of avoid using their methods (we are going > to rely a lot more on Qt since KF5, so we have to trust what Qt does). > > Kevin Ottens wrote: > Well, KUrlMimeData has some specific handling for KIO, which is a big > no-no for Qt. That's more what I had in mind... we don't need the metadata > part in that context though, so it's likely a moot point. > > That said we look for the application/x-kde4-urilist mimetype too (again > something we can't really push in Qt). David, any reason why we have this > extra mimetype? Doesn't really makes sense to me. > > David Faure wrote: > KUrlMimeData has some specific handling for KIO, and not just metadata, > but also shipping two lists of URLs: KIO urls and most-local urls. For > instance desktop:/foo and file://home/dfaure/Desktop/foo, the first one being > aimed at KIO-enabled apps and the second one at non-KIO-enabled apps. > > The code here simply checks for "there are urls", and lets QLineEdit do > the extraction. > This means that the most-local urls are going to be pasted, not the KIO > ones. > But that's not really specific to kurlrequester nor the > clear-before-insert feature... (e.g. konqueror's location bar would have the > same issue) > > WAIT ..... who determined that only KUrlRequester used the "url drop" > feature of KLineEdit? > That feature is ON by default, so looking for calls to > setUrlDropsEnabled(true) is definitely wrong. > Konqueror's location bar definitely uses that feature too. I suspect the > location bar in dolphin and the one in kfiledialog do as well, at least. > > It seems to me that putting the code into KUrlRequester is just wrong. It > should be an event filter that can be plugged onto any QLineEdit. And then it > can take care of both: clear before insert, and using KUrlMimeData to do the > URL extraction. > > Kevin Ottens wrote: > Hmmm... You're right, as I mentionned earlier that was my only concern > that url drops were enabled by default... But somehow I assumed that most of > our URL bars were KUrlRequester. Dunno what happened in my brain that day as > obviously it's not the case. They're either KComboBox or KLineEdit. > > OK... so if we want to make the url drops feature more reusable we should > make that feature an event filter in its own class as I initially proposed. > > But (and that's a strong but), I now realize that we never pushed toward > KUrlRequester to use QLineEdit or QComboBox. It can keep using KLineEdit or > KComboBox just fine. They're not getting deprecated, they'll be in > KCompletion (and the future KioWidgets will be able to use KCompletion just > fine IMO). > > It makes me think this patch should in fact be dropped. > > Albert Vaca Cintora wrote: > I see your point and agree with you. However I don't think that every > other class using this feature can keep using KLineEdit as before (some will > need to be ported to QLineEdit even if I was wrong and this is not the case > of KUrlRequester) and I don't think this feature is generic enough to send a > patch to Qt. I could then move it to a separate class (usable from wherever > we need) as Ervin suggested initially, but... do you feel it necessary or can > we just drop this feature from the apps we migrate to QLineEdit? > > Since you have a more comprehensive perspective of the project as I have, > if you think we should keep it I will write the patch to move it to an > independent class.
Right, it's kind of case by case basis indeed... Some might want just a QLineEdit with no completion and still get this drop behavior for URLs. So yes, please make a new patch to put that as a separate class usable on top of anything having a text property. Thanks in advance! - Kevin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111184/#review35019 ----------------------------------------------------------- On June 25, 2013, 10:46 a.m., Albert Vaca Cintora wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/111184/ > ----------------------------------------------------------- > > (Updated June 25, 2013, 10:46 a.m.) > > > Review request for KDE Frameworks. > > > Description > ------- > > KLineEdit and KComboBox had the property enableUrlDrops that was added to use > it in KUrlRequester. Since KUrlRequester is part of KIO, it can not use > Kde4Support and should be ported to use QLineEdit and QComboBox. As I said on > my email to the frameworks list, it was easier to implement this directly in > KUrlRequester than patching Qt, even though it was originally planned as a Qt > task. It was my intention to also remove that property from KLineEdit and > KComboBox, but finally I didn't include it in the patch (see Ervin's comment > in the mailing list). > > In the patch I have also added a TODO because I think it would be good to > detect any url without needing the mimetype to be set, but I have not done it > yet because it should be discussed first. > > (Reviewed by aacid and apol) > > > Diffs > ----- > > KDE5PORTING.html ba67bdc > kdeui/widgets/kcombobox.h ccb019d > kdeui/widgets/klineedit.h 7ac22f6 > kio/kfile/kurlrequester.cpp fa6b234 > > Diff: http://git.reviewboard.kde.org/r/111184/diff/ > > > Testing > ------- > > > Thanks, > > Albert Vaca Cintora > >
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
