Details
- Reviewers
drosca - Group Reviewers
Falkon - Commits
- R875:fb95cc81d608: Add support for Drag (on) and Drop (off) the bookmark toolbar
Diff Detail
- Repository
- R875 Falkon
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
| src/lib/bookmarks/bookmarkstoolbar.cpp | ||
|---|---|---|
| 306 | This should also call update() so you don't need to always call it after clearDropIndicator()- | |
| 319 | There is already code to draw drop indicator in ComboTabBar. It should be extracted from there to eg. QzTools and use it both here and in ComboTabBar. | |
| src/lib/bookmarks/bookmarkstoolbarbutton.cpp | ||
| 316 | You don't need to store that drag is in progress as drags are sync. | |
| 327 | How does dragging between windows work? It should use custom mimedata with BookmarkItem pointer (see TabModel). | |
| 331 | It starts drag on every mouse move event (with pressed mouse button)? Why is it calculating start drag distance then? | |
| src/lib/bookmarks/bookmarkitem.h | ||
|---|---|---|
| 31 ↗ | (On Diff #30701) | This is not needed. |
| src/lib/bookmarks/bookmarksmodel.cpp | ||
| 363 | Why does it still need pos? Index of item in layout is just index of BookmarkItem in parent->children. | |
| src/lib/bookmarks/bookmarksmodel.h | ||
| 121 | No QPointer. | |
| src/lib/bookmarks/bookmarkstoolbar.cpp | ||
| 34 | You already have it in static BookmarksButtonMimeData::mimeType() | |
| 283 | Just set m_dragPos here and you can remove calcDropRow if you do the calculation in paintEvent instead. | |
| 315 | This needs to call QWidget::paintEvent too. | |
| src/lib/bookmarks/bookmarkstoolbarbutton.cpp | ||
| 311 | QDrag *drag | |
| 319 | This still needs to call mouseMoveEvent of parent class. if ((event->pos() - m_dragStartPosition).manhattanLength() < QApplication::startDragDistance()) {
QPushButton::mouseMoveEvent(event);
return;
} | |
| src/lib/bookmarks/bookmarkstoolbarbutton.h | ||
| 70 | QMouseEvent *event | |
| src/lib/tools/qztools.h | ||
| 100 | const QRect &r and rename parent -> widget. | |
| src/lib/bookmarks/bookmarkitem.h | ||
|---|---|---|
| 31 ↗ | (On Diff #30701) | no, this is required |
| src/lib/bookmarks/bookmarkstoolbarbutton.h | ||
|---|---|---|
| 70 | mostly everywhere in falkon headers the syntax I found is type* var, this change is still confusing to me :-| | |
| src/lib/bookmarks/bookmarkitem.h | ||
|---|---|---|
| 31 ↗ | (On Diff #30701) | Why? |
| src/lib/bookmarks/bookmarkstoolbar.cpp | ||
| 265 | initialIndex, also it can be const. | |
| 272 | const QUrl + make the variables const in other places too if possible. | |
| src/lib/bookmarks/bookmarkstoolbarbutton.h | ||
| 70 | Yeah, that's true for old code. New code should be using KDE coding style. | |
| src/lib/tools/qztools.h | ||
| 100 | const-ref const QRect &r | |
| src/lib/bookmarks/bookmarkitem.h | ||
|---|---|---|
| 31 ↗ | (On Diff #30701) | sure not needed, I was just overthinking |
| src/lib/bookmarks/bookmarkitem.h | ||
|---|---|---|
| 31 ↗ | (On Diff #30701) | But that was meant also to the QObject subclass, it's not needed. |
| src/lib/bookmarks/bookmarkstoolbar.cpp | ||
| 314 | What if button == nullptr, you check for it few lines bellow this but not here. | |
| 330 | It has to call QWidget::paintEvent(p) as the first thing in this method, otherwise it could just paint over the drop indicator. | |
Asserts when trying to drop bookmark on free space at the end of toolbar.
ASSERT: "row >= 0" in file src/lib/bookmarks/bookmarksmodel.cpp, line 51
Also would be nice to have some indication on button when dropping the bookmark into folder, maybe setDown(true) to make it simple?