search

lxqt / lxqt-panel

The LXQt desktop panel
https://lxqt-project.org
GNU Lesser General Public License v2.1
193 stars 135 forks source link

FancyMenu: support favorites reordening by drag drop #2013

Closed gfgit closed 10 months ago

gfgit commented 10 months ago

Closes #1988

gfgit commented 10 months ago

Well actually we could reuse text/uri-list mime instead of custom application/x-lxqtfavoritesdragrow but then it would also accept foreign drops. They get rejected after by comparing with current favorites. The original row is calculated on drop so it does not need to be stored inside drag mime data.

tsujan commented 10 months ago

I haven't started to review this PR because I've found a serious issue with DNDing items from Fancy Menu into PCManFM-Qt under Wayland (haven't tried X11). As I should first see where the cause is (Qt5, Qt in general, or just Fancy Menu?), I have to delay the review for a while. I'll report it when I find the time to investigate it but, IMO, fixing it has priority.

tsujan commented 10 months ago

OK, I tested under X11: there's also a problem there, with the same cause but a slightly different effect. The cause is that Fancy Menu isn't closed on DNDing; the effect is that the focus is locked by Fancy Menu, until somewhere is clicked.

I emphasize that this issue exits in the git source (and so, not related to the current PR).

tsujan commented 10 months ago

In addition to the above reviews, please make sure you've checked that old and new rows/positions are always valid, i.e., they are greater than -1 and less than the number of items. You could ignore this comment if you've already done it (I didn't check).

gfgit commented 10 months ago

In addition to the above reviews, please make sure you've checked that old and new rows/positions are always valid, i.e., they are greater than -1 and less than the number of items. You could ignore this comment if you've already done it (I didn't check).

Well I've read Qt default implementation and row == -1 is interpreted as a drop below last row. Currently to drop at last row you have to drag close to last item, you cannot just drop is the empty area below it. In my opinion this is not bad, also because row == -1 might be received in other situations where drop is not close to last row so it would be weird if item ended up there. But this behavior is just one line change so I can implement it if it's wanted.

tsujan commented 10 months ago

Done by entirely removing mFavorites

Yes, that's better; mFavorites seemed like a duplication. Will review the new code soon.

But this behavior is just one line change so I can implement it if it's wanted.

I just meant safeguards against crashes. If no crash can happen, this is OK, especially after the removal of mFavorites.

stefonarch commented 10 months ago

Looks working fine now at first test.

tsujan commented 10 months ago

@stefonarch If you find no problem in what this PR is supposed to do, please merge it.