From kde-devel Fri Apr 28 00:47:28 2006 From: "Will Entriken" Date: Fri, 28 Apr 2006 00:47:28 +0000 To: kde-devel Subject: Re: KDE4todo #50 Message-Id: X-MARC-Message: https://marc.info/?l=kde-devel&m=114618528611986 MIME-Version: 1 Content-Type: multipart/mixed; boundary="------=_Part_16550_27238405.1146185248567" ------=_Part_16550_27238405.1146185248567 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Content-Disposition: inline Huge cleanup to kio::bookmarks Summary: * dptr is gone, the .h is slated for deletion * KBookmarkManagerPrivate is gone, its members were promoted to KBookmarkMa= nager * RMB constructors are fixed so that: * KBookmarkmenu, KBookmarkbar and RMB are decoupled/unfriended * it compiles... with a million warnings about deprecation Were all of these pointers and hashes and private magic classes all used just to preserve binary compatibility while stuffing in more member variables? If so, you think they would have a standard way of doing that? I would like to test this with something before committing it, is kdeui ready for this yet? Cheers, Will Entriken On 4/27/06, Daniel Teske wrote: > > > For the bookmark menu, I am using a slightly better constructor: > > RMB(KBookmarkMenu *target) > Well I'd say worse. RMB and KBookmarkMenu are too tightly coupled. RMB sh= ouldn't directly access member variables of KBookmarkMenu. > > > I need to use the KBookmarkMenu * because (QMenu) > > contextMenu->addAction(...) uses recv all over the place in > > kbookmarkmenu.cc, so it's not as cool as the bookmarkbar solution. > Well you have to set recv inside the other RMB constructor, too. > Refactoring that part can wait a little bit. But it shouldn't use recv th= ere. > > > As far as replacing all dPtrs: Without understanding much about the > > code, I intend to replace them with QHashes. I think I'm starting to > > understand this code, and at that, I would recommend that > > KBookmarkMenu get a RMB *m_RMB and KBookmarkManager get a > > KBookmarkManagerPrivate * m_kBookmarkManagerPrivate. What are your > > thoughts here? > > Right. And then replace any rmbSelf(this) with m_rmb. > And the constructor should then set his data and not that of rmbSelf(recv= ) > When it's all done, dptrtemplate.h should be unused. > > > Also, I'm making assumptions about KBookmarkMenu::d working the same > > as KBookmarkBar::d. However, the former is in the header, but not > > implemented !?!?! > > Right. KBookmarkMenuPrivate isn't defined anywhere. Uhm, I'd say bitrot. = For now add the pointer to RMB to KBookmarkMenu > > > 2) use d->m_rmb instead of rmbSelf(this) > Use m_rmb instead of rmbSelf(this). You'll need to add a declaration for = m_rmb to KBookmarkMenu. > > daniel > ------=_Part_16550_27238405.1146185248567 Content-Type: application/octet-stream; name=bookmarkscleanup.patch Content-Transfer-Encoding: 7bit X-Attachment-Id: f_emjtbir1 Content-Disposition: attachment; filename="bookmarkscleanup.patch" Index: kbookmarkbar.cc =================================================================== --- kbookmarkbar.cc (revision 533892) +++ kbookmarkbar.cc (working copy) @@ -263,6 +263,7 @@ } } +//TODO: kill me static KAction* findPluggedAction(const QList& actions, KToolBar *tb, int id) { /*for ( QList::const_iterator it = actions.begin(), end = actions.end() ; it != end ; ++it ) { @@ -403,34 +404,48 @@ // TODO *** generic rmb improvements *** // don't *ever* show the rmb on press, always relase, possible??? - -void RMB::begin_rmb_action(KBookmarkBar *self) +RMB::RMB(QString parentAddress, QString highlightedAddress, + KBookmarkManager *pManager, KBookmarkOwner *pOwner) +: m_parentAddress(parentAddress), m_highlightedAddress(highlightedAddress), + m_pManager(pManager), m_pOwner(pOwner), m_parentMenu(0) { - if ( !self->d->m_rmb ) - self->d->m_rmb = new RMB; - RMB *s = self->d->m_rmb; - s->recv = self; - s->m_parentAddress = self->parentAddress(); - s->s_highlightedAddress = self->d->m_highlightedAddress; // rename in RMB - s->m_pManager = self->m_pManager; - s->m_pOwner = self->m_pOwner; - s->m_parentMenu = 0; } void KBookmarkBar::slotRMBActionEditAt( int val ) -{ RMB::begin_rmb_action(this); d->m_rmb->slotRMBActionEditAt( val ); } +{ + delete d->m_rmb; + d->m_rmb = new RMB(parentAddress(), d->m_highlightedAddress, m_pManager, m_pOwner); + d->m_rmb->slotRMBActionEditAt( val ); +} void KBookmarkBar::slotRMBActionProperties( int val ) -{ RMB::begin_rmb_action(this); d->m_rmb->slotRMBActionProperties( val ); } +{ + delete d->m_rmb; + d->m_rmb = new RMB(parentAddress(), d->m_highlightedAddress, m_pManager, m_pOwner); + d->m_rmb->slotRMBActionProperties( val ); +} void KBookmarkBar::slotRMBActionInsert( int val ) -{ RMB::begin_rmb_action(this); d->m_rmb->slotRMBActionInsert( val ); } +{ + delete d->m_rmb; + d->m_rmb = new RMB(parentAddress(), d->m_highlightedAddress, m_pManager, m_pOwner); + d->m_rmb->slotRMBActionInsert( val ); +} + void KBookmarkBar::slotRMBActionRemove( int val ) -{ RMB::begin_rmb_action(this); d->m_rmb->slotRMBActionRemove( val ); } +{ + delete d->m_rmb; + d->m_rmb = new RMB(parentAddress(), d->m_highlightedAddress, m_pManager, m_pOwner); + d->m_rmb->slotRMBActionRemove( val ); +} void KBookmarkBar::slotRMBActionCopyLocation( int val ) -{ RMB::begin_rmb_action(this); d->m_rmb->slotRMBActionCopyLocation( val ); } +{ + delete d->m_rmb; + d->m_rmb = new RMB(parentAddress(), d->m_highlightedAddress, m_pManager, m_pOwner); + d->m_rmb->slotRMBActionCopyLocation( val ); +} bool KBookmarkBar::eventFilter( QObject *o, QEvent *e ) { @@ -451,7 +466,8 @@ { d->m_highlightedAddress = _a->property("address").toString(); KBookmark bookmark = m_pManager->findByAddress( d->m_highlightedAddress ); - RMB::begin_rmb_action(this); + delete d->m_rmb; + d->m_rmb = new RMB(parentAddress(), d->m_highlightedAddress, m_pManager, m_pOwner); KMenu *pm = new KMenu; d->m_rmb->fillContextMenu( pm, d->m_highlightedAddress, 0 ); emit aboutToShowContextMenu( d->m_rmb->atAddress( d->m_highlightedAddress ), pm ); Index: kbookmarkmenu.cc =================================================================== --- kbookmarkmenu.cc (revision 533892) +++ kbookmarkmenu.cc (working copy) @@ -50,7 +50,6 @@ #include #include #include -#include static QString makeTextNodeMod(KBookmark bk, const QString &m_nodename, const QString &m_newText) { QDomNode subnode = bk.internalElement().namedItem(m_nodename); @@ -157,7 +156,7 @@ } } -QString KBookmarkMenu::s_highlightedAddress; +QString KBookmarkMenu::m_highlightedAddress; QString KBookmarkMenu::s_highlightedImportType; QString KBookmarkMenu::s_highlightedImportLocation; @@ -166,8 +165,8 @@ if (qobject_cast(action) || qobject_cast(action)) { - s_highlightedAddress = action->property("address").toString(); - //kDebug() << "KBookmarkMenu::slotActionHighlighted" << s_highlightedAddress << endl; + m_highlightedAddress = action->property("address").toString(); + //kDebug() << "KBookmarkMenu::slotActionHighlighted" << m_highlightedAddress << endl; } else if (qobject_cast(action)) { @@ -176,7 +175,7 @@ } else { - s_highlightedAddress.clear(); + m_highlightedAddress.clear(); s_highlightedImportType.clear(); s_highlightedImportLocation.clear(); } @@ -186,34 +185,24 @@ /********************************************************************/ /********************************************************************/ -// TODO use d pointer instead - or add a pointer for the RMB instance. - -class KBookmarkMenuRMBAssoc : public dPtrTemplate { }; -template<> QHash* dPtrTemplate::d_ptr = 0; - -static RMB* rmbSelf(KBookmarkMenu *m) { return KBookmarkMenuRMBAssoc::d(m); } - // TODO check via dcop before making any changes to the bookmarks file??? -void RMB::begin_rmb_action(KBookmarkMenu *self) +RMB::RMB(KBookmarkMenu *target, QString parentAddress, QString highlightedAddress, + KBookmarkManager *pManager, KBookmarkOwner *pOwner, QWidget *parentMenu) +: recv(target), m_parentAddress(parentAddress), m_highlightedAddress(highlightedAddress), + m_pManager(pManager), m_pOwner(pOwner), m_parentMenu(parentMenu) { - RMB *s = rmbSelf(self); - s->recv = self; - s->m_parentAddress = self->m_parentAddress; - s->s_highlightedAddress = KBookmarkMenu::s_highlightedAddress; - s->m_pManager = self->m_pManager; - s->m_pOwner = self->m_pOwner; - s->m_parentMenu = self->m_parentMenu; } + bool RMB::invalid( int val ) { bool valid = true; if (val == 1) - s_highlightedAddress = m_parentAddress; + m_highlightedAddress = m_parentAddress; - if (s_highlightedAddress.isNull()) + if (m_highlightedAddress.isNull()) valid = false; return !valid; @@ -228,14 +217,14 @@ void KBookmarkMenu::slotAboutToShowContextMenu( KMenu*, QAction*, QMenu* contextMenu ) { - //kDebug(7043) << "KBookmarkMenu::slotAboutToShowContextMenu" << s_highlightedAddress << endl; - if (s_highlightedAddress.isNull()) + //kDebug(7043) << "KBookmarkMenu::slotAboutToShowContextMenu" << m_highlightedAddress << endl; + if (m_highlightedAddress.isNull()) { KMenu::contextMenuFocus()->hideContextMenu(); return; } contextMenu->clear(); - fillContextMenu( contextMenu, s_highlightedAddress, 0 ); + fillContextMenu( contextMenu, m_highlightedAddress, 0 ); } void RMB::fillContextMenu( QMenu* contextMenu, const QString & address, int val ) @@ -292,20 +281,20 @@ void RMB::slotRMBActionEditAt( int val ) { - kDebug(7043) << "KBookmarkMenu::slotRMBActionEditAt" << s_highlightedAddress << endl; + kDebug(7043) << "KBookmarkMenu::slotRMBActionEditAt" << m_highlightedAddress << endl; if (invalid(val)) { hidePopup(); return; } - KBookmark bookmark = atAddress(s_highlightedAddress); + KBookmark bookmark = atAddress(m_highlightedAddress); - m_pManager->slotEditBookmarksAtAddress( s_highlightedAddress ); + m_pManager->slotEditBookmarksAtAddress( m_highlightedAddress ); } void RMB::slotRMBActionProperties( int val ) { - kDebug(7043) << "KBookmarkMenu::slotRMBActionProperties" << s_highlightedAddress << endl; + kDebug(7043) << "KBookmarkMenu::slotRMBActionProperties" << m_highlightedAddress << endl; if (invalid(val)) { hidePopup(); return; } - KBookmark bookmark = atAddress(s_highlightedAddress); + KBookmark bookmark = atAddress(m_highlightedAddress); QString folder = bookmark.isGroup() ? QString() : bookmark.url().pathOrURL(); KBookmarkEditDialog dlg( bookmark.fullText(), folder, @@ -332,7 +321,7 @@ void RMB::slotRMBActionInsert( int val ) { - kDebug(7043) << "KBookmarkMenu::slotRMBActionInsert" << s_highlightedAddress << endl; + kDebug(7043) << "KBookmarkMenu::slotRMBActionInsert" << m_highlightedAddress << endl; if (invalid(val)) { hidePopup(); return; } QString url = m_pOwner->currentURL(); @@ -345,7 +334,7 @@ if (title.isEmpty()) title = url; - KBookmark bookmark = atAddress( s_highlightedAddress ); + KBookmark bookmark = atAddress( m_highlightedAddress ); // TODO use unique title @@ -368,10 +357,10 @@ void RMB::slotRMBActionRemove( int val ) { - //kDebug(7043) << "KBookmarkMenu::slotRMBActionRemove" << s_highlightedAddress << endl; + //kDebug(7043) << "KBookmarkMenu::slotRMBActionRemove" << m_highlightedAddress << endl; if (invalid(val)) { hidePopup(); return; } - KBookmark bookmark = atAddress( s_highlightedAddress ); + KBookmark bookmark = atAddress( m_highlightedAddress ); bool folder = bookmark.isGroup(); if (KMessageBox::warningContinueCancel( @@ -394,10 +383,10 @@ void RMB::slotRMBActionCopyLocation( int val ) { - //kDebug(7043) << "KBookmarkMenu::slotRMBActionCopyLocation" << s_highlightedAddress << endl; + //kDebug(7043) << "KBookmarkMenu::slotRMBActionCopyLocation" << m_highlightedAddress << endl; if (invalid(val)) { hidePopup(); return; } - const KBookmark bookmark = atAddress( s_highlightedAddress ); + const KBookmark bookmark = atAddress( m_highlightedAddress ); if ( !bookmark.isGroup() ) { @@ -420,26 +409,53 @@ void KBookmarkMenu::fillContextMenu( QMenu* contextMenu, const QString & address, int val ) { - RMB::begin_rmb_action(this); - rmbSelf(this)->fillContextMenu(contextMenu, address, val); - emit aboutToShowContextMenu( rmbSelf(this)->atAddress(address), contextMenu); - rmbSelf(this)->fillContextMenu2(contextMenu, address, val); + delete m_rmb; + m_rmb = new RMB(this, m_parentAddress, KBookmarkMenu::m_highlightedAddress, + m_pManager, m_pOwner, m_parentMenu); + m_rmb->fillContextMenu(contextMenu, address, val); + emit aboutToShowContextMenu( m_rmb->atAddress(address), contextMenu); + m_rmb->fillContextMenu2(contextMenu, address, val); } void KBookmarkMenu::slotRMBActionEditAt( int val ) -{ RMB::begin_rmb_action(this); rmbSelf(this)->slotRMBActionEditAt( val ); } +{ + delete m_rmb; + m_rmb = new RMB(this, m_parentAddress, KBookmarkMenu::m_highlightedAddress, + m_pManager, m_pOwner, m_parentMenu); + m_rmb->slotRMBActionProperties( val ); +} void KBookmarkMenu::slotRMBActionProperties( int val ) -{ RMB::begin_rmb_action(this); rmbSelf(this)->slotRMBActionProperties( val ); } +{ + delete m_rmb; + m_rmb = new RMB(this, m_parentAddress, KBookmarkMenu::m_highlightedAddress, + m_pManager, m_pOwner, m_parentMenu); + m_rmb->slotRMBActionProperties( val ); +} void KBookmarkMenu::slotRMBActionInsert( int val ) -{ RMB::begin_rmb_action(this); rmbSelf(this)->slotRMBActionInsert( val ); } +{ + delete m_rmb; + m_rmb = new RMB(this, m_parentAddress, KBookmarkMenu::m_highlightedAddress, + m_pManager, m_pOwner, m_parentMenu); + m_rmb->slotRMBActionProperties( val ); +} void KBookmarkMenu::slotRMBActionRemove( int val ) -{ RMB::begin_rmb_action(this); rmbSelf(this)->slotRMBActionRemove( val ); } +{ + delete m_rmb; + m_rmb = new RMB(this, m_parentAddress, KBookmarkMenu::m_highlightedAddress, + m_pManager, m_pOwner, m_parentMenu); + m_rmb->slotRMBActionProperties( val ); +} void KBookmarkMenu::slotRMBActionCopyLocation( int val ) -{ RMB::begin_rmb_action(this); rmbSelf(this)->slotRMBActionCopyLocation( val ); } +{ + delete m_rmb; + m_rmb = new RMB(this, m_parentAddress, KBookmarkMenu::m_highlightedAddress, + m_pManager, m_pOwner, m_parentMenu); + m_rmb->slotRMBActionProperties( val ); +} void KBookmarkMenu::slotBookmarksChanged( const QString & groupAddress ) { Index: kbookmarkbar.h =================================================================== --- kbookmarkbar.h (revision 533892) +++ kbookmarkbar.h (working copy) @@ -43,7 +43,6 @@ class KIO_EXPORT KBookmarkBar : public QObject { Q_OBJECT - friend class RMB; public: /** * Fills a bookmark toolbar Index: dptrtemplate.h =================================================================== --- dptrtemplate.h (revision 533892) +++ dptrtemplate.h (working copy) @@ -1,3 +1,5 @@ +/* DELETEME!!! I am a massive binary compatibility hack from KDE3 */ + // -*- c-basic-offset:4; indent-tabs-mode:nil -*- // vim: set ts=4 sts=4 sw=4 et: /* This file is part of the KDE project @@ -24,9 +26,11 @@ #include +/* A lazily-created hash from Instance to PrivateData */ template class dPtrTemplate { public: + /* get hash[instance] */ static PrivateData* d( const Instance* instance ) { if ( !d_ptr ) { @@ -40,10 +44,11 @@ } return ret; } + /* remove hash[instance] */ static void delete_d( const Instance* instance ) { if ( d_ptr ) - delete d_ptr->take( instance ); + delete d_ptr->remove( instance ); } private: static void cleanup_d_ptr() Index: kbookmarkmenu.h =================================================================== --- kbookmarkmenu.h (revision 533892) +++ kbookmarkmenu.h (working copy) @@ -33,6 +33,7 @@ #include "kbookmark.h" #include "kbookmarkmanager.h" +#include "kbookmarkmenu_p.h" class QString; class QMenu; @@ -77,7 +78,6 @@ { Q_OBJECT friend class KBookmarkMenuNSImporter; - friend class RMB; public: /** * Fills a bookmark menu @@ -215,12 +215,12 @@ */ QString m_parentAddress; - class KBookmarkMenuPrivate* d; - // TODO make non static! - static QString s_highlightedAddress; + static QString m_highlightedAddress; static QString s_highlightedImportLocation; static QString s_highlightedImportType; + + RMB *m_rmb; }; /** Index: kbookmarkmanager.cc =================================================================== --- kbookmarkmanager.cc (revision 533892) +++ kbookmarkmanager.cc (working copy) @@ -40,21 +40,6 @@ #include #include -#include "dptrtemplate.h" - -class KBookmarkManagerPrivate : public dPtrTemplate { -public: - QString m_editorCaption; - bool m_browserEditor; -}; -template<> QHash* dPtrTemplate::d_ptr = 0; - -KBookmarkManagerPrivate* KBookmarkManager::dptr() const { - return KBookmarkManagerPrivate::d( this ); -} - -// TODO - clean this stuff up by just using the above dptrtemplate? - class KBookmarkManagerList : public QList { public: @@ -607,17 +592,17 @@ void KBookmarkManager::setEditorOptions( const QString& caption, bool browser ) { - dptr()->m_editorCaption = caption; - dptr()->m_browserEditor = browser; + m_editorCaption = caption; + m_browserEditor = browser; } void KBookmarkManager::slotEditBookmarks() { KProcess proc; proc << QLatin1String("keditbookmarks"); - if (!dptr()->m_editorCaption.isNull()) - proc << QLatin1String("--customcaption") << dptr()->m_editorCaption; - if (!dptr()->m_browserEditor) + if (!m_editorCaption.isNull()) + proc << QLatin1String("--customcaption") << m_editorCaption; + if (!m_browserEditor) proc << QLatin1String("--nobrowser"); proc << m_bookmarksFile; proc.start(KProcess::DontCare); Index: kbookmarkmanager.h =================================================================== --- kbookmarkmanager.h (revision 533892) +++ kbookmarkmanager.h (working copy) @@ -276,8 +276,8 @@ static KBookmarkManagerList* s_pSelf; bool m_showNSBookmarks; -private: - class KBookmarkManagerPrivate* dptr() const; + QString m_editorCaption; + bool m_browserEditor; }; /** Index: kbookmarkmenu_p.h =================================================================== --- kbookmarkmenu_p.h (revision 533892) +++ kbookmarkmenu_p.h (working copy) @@ -190,12 +190,15 @@ static KBookmarkSettings *self(); }; +/* Right mouse button */ class RMB { public: - // ##### DF: How about two normal constructors instead?? - static void begin_rmb_action(KBookmarkMenu *); - static void begin_rmb_action(KBookmarkBar *); + RMB(QString parentAddress, QString highlightedAddress, + KBookmarkManager *pManager, KBookmarkOwner *pOwner); + RMB(KBookmarkMenu *target, QString parentAddress, QString highlightedAddress, + KBookmarkManager *pManager, KBookmarkOwner *pOwner, QWidget *parentMenu); + bool invalid( int val ); KBookmark atAddress(const QString & address); void fillContextMenu( QMenu* contextMenu, const QString & address, int val ); @@ -209,7 +212,7 @@ public: QObject *recv; KBookmarkManager *m_pManager; - QString s_highlightedAddress; + QString m_highlightedAddress; QString m_parentAddress; KBookmarkOwner *m_pOwner; QWidget *m_parentMenu; ------=_Part_16550_27238405.1146185248567 Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline >> Visit http://mail.kde.org/mailman/listinfo/kde-devel#unsub to unsubscribe << ------=_Part_16550_27238405.1146185248567--