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 | 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?