-
Notifications
You must be signed in to change notification settings - Fork 114
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Align icons to grid on desktop #97
Comments
+1 |
This will change the UI. Leave this for 1.0. |
This commit addresses several related issues in icon view and/or on desktop: (1) Some items with long names overlapped, such that selecting one of them made others be selected too (lxqt/pcmanfm-qt#251 and lxqt/lxqt#768); (2) Sometimes file names weren't completely inside selection rectangles. (3) Not being aligned to the grid (lxqt/pcmanfm-qt#97), desktop items with custom positions could overlap and even occupy the same locations (lxqt/lxqt#791). (4) The positions of such items didn't get updated when, for example, a left panel was removed/created. (5) The deleted/renamed files weren't removed from the custom postion list. As for the first and second issues, some size miscalculations are fixed in FolderItemDelegate and a 6-px margin for cells plus a 2-px margin for selection rects is given to FolderView's grid size. Similar corrections are made for DesktopItemDelegate too. Desktop grid alignment and custom position update are forced by using QListView::Snap and modifying DesktopWindow::loadItemPositions(), DesktopWindow::relayoutItems() and DesktopWindow::onIndexesMoved(). Finally, the 5th issue was because removal from the list was dependent on file existence, while the file was already deleted/renamed.
Using a grid was introduced as default behaviour in #252 which solved several issues like selection of multiple objects by clicking a single one. |
@pmattern |
@pmattern |
I think it's safe to close. At least for me, unaligned icons are messy. Anyway, I think there is a issue with the grid on desktop: the grid does not stretch 100% vertically, and a gap is created in the bottom where you can't drag a icon to. This can be easy tested by using @tsujan can you reproduce it? |
@paulolieuthier |
That's exactly the case, and I understand my screen's height is not a multiple of the icon height. I just don't think it's OK. IMO, spacing must be added between the icons to remove the gap in the bottom. |
That seems to be a good idea. Since pcmanfm-qt partly uses size settings of libfm-qt, I should see if the implementation of this idea is possible without changing the latter. It might be a lot of work but I'll consider it. Thanks for the suggestion! |
Thanks! |
@paulolieuthier Or you could just send a screenshot of your desktop when, at least, its first column is fully occupied by icons. |
Thanks! Most of that gap will be covered -- only the 12-px default plus a few px will remain. I'll make the PR's after more testing and considering various aspects of the changes, whose byproduct is the possibility of adding settings to pcmanfm-qt for spacing. |
That's great news, thanks again. Just for curiosity, what is the 12px margin for? |
PCMan had considered a 12-px margin for the whole work area |
👍 |
@paulolieuthier What do you think about this algorithm: "If an extra icon can be added to the bottom by decreasing the vertical cell margin, do so; otherwise try to spread the icons to cover the bottom gap as far as possible!" The default cell margin is 3 px (which gives a 6-px spacing). Assuming a minimum of 1 px for the margin and judging by your screenshot, I think you might get 24=6x2x(3-1) extra pixels at the bottom. I don't know if that's enough for an extra icon in your case; I just wanted to give a picture of the above algorithm for you (and anyone interested) to think about it. In simpler words, it means: "First try to tighten columns but if that doesn't help, try to spread icons!." |
I think this sounds like a good idea. However, if the spacing will become an option in pcmanfm-qt, than we shouldn't change it to fit one more icon. Does it make sense? In my screenshot, it looks like there is enough space for another icon, doesn't it? It's probably a matter of a few missing pixels for another icon to fit. If we spread the icons, they will probably be way too far. I think the right way to go is to keep the margin fixed, but smaller. With an option for the spacing, the user may increase it if he thinks the icons are too close (the option would set the minimum spacing). What do you think? |
Exactly! Neither should we spread the icons. A few hours ago, I tried the above method and got good results. In the image below, the left half shows desktop icons arranged as before, while the right half shows the same icons with the algorithm applied: one row is added to the bottom and there's no large gap anymore. So far, so good. If a setting is added for margins, I'll set a condition in the code for this optimization to be used only with the default settings because when the user chooses a specific margin, he wants it unchanged -- as you mentioned. |
Good results. I'm looking forward to it. Just to make it clear: IMO there must never be a gap. The option would be to set the minimum spacing, to avoid getting the icons too close. But I guess this is a matter of taste. |
OK, we'll discuss that when an option is going to be added, perhaps under lxqt/lxqt#786 . For now, I try to optimize the code and do more testing. |
@paulolieuthier |
Sure, I'll have some time tomorrow. |
@paulolieuthier Thank you! I was going to ask you to add a screenshot but you already did :)
Desktop is bounded from below while the icons snap to the grid starting from above. On the other hand, to change the grid (cell) dimensions, we have to change its margins. So, we can change the height or width of all cells only by even numbers. Hence a bit of asymmetry is unavoidable. However, now the best use is made out of the available space. If PCMan accepts the PR's, I'll start to work on spacing options after discussing some technicalities at lxqt/lxqt#786. |
Great. |
Closed by #294 |
The margin option will be discussed at lxqt/lxqt#786. |
There should be an option for that.
The text was updated successfully, but these errors were encountered: