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
- Branch
- patch-1
- Lint
No Linters Available - Unit
No Unit Test Coverage
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 ↗ | (On Diff #30701) | Why does it still need pos? Index of item in layout is just index of BookmarkItem in parent->children. |
src/lib/bookmarks/bookmarksmodel.h | ||
123 ↗ | (On Diff #30701) | No QPointer. |
src/lib/bookmarks/bookmarkstoolbar.cpp | ||
34 ↗ | (On Diff #30701) | You already have it in static BookmarksButtonMimeData::mimeType() |
284 ↗ | (On Diff #30701) | Just set m_dragPos here and you can remove calcDropRow if you do the calculation in paintEvent instead. |
316 ↗ | (On Diff #30701) | This needs to call QWidget::paintEvent too. |
src/lib/bookmarks/bookmarkstoolbarbutton.cpp | ||
307 ↗ | (On Diff #30701) | QDrag *drag |
315 ↗ | (On Diff #30701) | 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 ↗ | (On Diff #30701) | QMouseEvent *event |
src/lib/tools/qztools.h | ||
100 ↗ | (On Diff #30701) | 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 ↗ | (On Diff #30701) | 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 | ||
251 | initialIndex, also it can be const. | |
258 | const QUrl + make the variables const in other places too if possible. | |
src/lib/bookmarks/bookmarkstoolbarbutton.h | ||
70 ↗ | (On Diff #30701) | Yeah, that's true for old code. New code should be using KDE coding style. |
src/lib/tools/qztools.h | ||
100 ↗ | (On Diff #30701) | 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 | ||
315 ↗ | (On Diff #30701) | What if button == nullptr, you check for it few lines bellow this but not here. |
331 ↗ | (On Diff #30701) | 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?