GridView: current_grid_key → set_page#699
Conversation
leolost2605
left a comment
There was a problem hiding this comment.
I don't know a lot about the code base but makes sense to me :) Just a few comments
Co-authored-by: Leonhard <leo.kargl@proton.me>
Co-authored-by: Leonhard <leo.kargl@proton.me>
Co-authored-by: Leonhard <leo.kargl@proton.me>
This comment was marked as resolved.
This comment was marked as resolved.
|
@danirabbit yes thank you! |
|
@leolost2605 okay this branch now only contains the final change from |
src/SlingshotView.vala
Outdated
| @@ -264,9 +264,9 @@ public class Slingshot.SlingshotView : Gtk.Bin, UnityClient { | |||
| var key = Gdk.keyval_name (keyval).replace ("KP_", ""); | |||
| int page = int.parse (key); | |||
| if (page < 0 || page == 9) { | |||
There was a problem hiding this comment.
| if (page < 0 || page == 9) { | |
| if (page <= 0 || page == 9) { |
I think this should also trigger at 0?
There was a problem hiding this comment.
If we changed that to = 0 then it would go the last page on 0 instead of the first page. I'm not sure that's expected? Unless we change also 9 to go to page 9 and 0 to be last page?
There was a problem hiding this comment.
Yeah I was a bit confused because I missed that on master the current grid key was clamped. I thought it was intended that 0 was last page because it's all the way to the right but I didn't look very close. The main thing why I mentioned it is because afaict with this PR we would then set_page (-1).
There was a problem hiding this comment.
Yeah which afaict still works the same as in master. Is it okay if we consider this separately since I didn't change the behavior?
There was a problem hiding this comment.
Hmm maybe I'm doing something wrong but for me on master alt + 0 would switch to the first page and with this branch it doesn't do anything. And afaict what's happening is that we overflow the uint because we have -1, which causes nth_data to return null because our list doesn't have uint.max entries. And even if this would somehow be handled and then switch to the first page I'm not sure relying on an overflow is good practice (?) so IMO we should handle this explicitly. Whether that's going to the first or last page I really don't have a strong opinion about but ig first page makes sense to stay in line with master for now.
There was a problem hiding this comment.
@leolost2605 got it! Made 0 and 9 explicit so we're not relying on the math here. Also I moved the conditional for the view mode check, so it should be easier to read. Might want to review with hide whitespace changes
leolost2605
left a comment
There was a problem hiding this comment.
LGTM thanks for the patience :)
|
@leolost2605 thank you for the thorough review! |
Replace
current_grid_keyproperty with aset_pagefunction. Changes the names of public functions to specify "page"This also changes the index to be 0 based