Skip to content

Jeremypw/gtk4 prep/bookmark list box cleanup#2540

Merged
danirabbit merged 5 commits intomainfrom
jeremypw/gtk4-prep/BookmarkListBox-cleanup
Mar 8, 2025
Merged

Jeremypw/gtk4 prep/bookmark list box cleanup#2540
danirabbit merged 5 commits intomainfrom
jeremypw/gtk4-prep/BookmarkListBox-cleanup

Conversation

@jeremypw
Copy link
Contributor

@jeremypw jeremypw commented Mar 7, 2025

Use equivalent code that will work in Gtk4 as well as Gtk3 as far as possible

@jeremypw jeremypw marked this pull request as ready for review March 7, 2025 16:20
Copy link
Member

@danirabbit danirabbit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a tiny comment/question/nitpick otherwise this LGTM

Comment on lines +198 to +199
int index = 0;
Gtk.ListBoxRow? row = list_box.get_row_at_index (index++);
Copy link
Member

@danirabbit danirabbit Mar 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You start the index at 0 but then immediately make it 1 here. That seems a bit overcomplicated? Why not just start the index where you need it to start? 😅 Also does the row index start at 1? Do we not start at 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless I'm mistaken, when ++ is post-fixed it operates after the variable is used so the current code is equivalent to

Gtk.ListBoxRow? row = list_box.get_row_at_index (index);
index = index + 1;

(when the operator is pre-fixed it increments the variable before using it)

I'll double-check though and see if there is a clearer, equally concise way.

@jeremypw
Copy link
Contributor Author

jeremypw commented Mar 8, 2025

@danirabbit
From Vala doumentation:

Increment and decrement operations with implicit assignment. These take just one argument, which must be an identifier of a simple data type. The value will be changed and assigned back to the identifier. These operators may be placed in either prefix or postfix positions - with the former the evaluated value of the statement will be the newly calculated value, with the latter the original value is returned.

So with the postfix operator the lookup expression uses the original (pre-incremented) value as I expected.

@jeremypw
Copy link
Contributor Author

jeremypw commented Mar 8, 2025

Now used the pre-fix operator which requires the same number of lines and is clearer. Not sure why I used the post-fix tbh

@jeremypw jeremypw requested a review from danirabbit March 8, 2025 13:11
@danirabbit danirabbit merged commit 6cca93d into main Mar 8, 2025
4 checks passed
@danirabbit danirabbit deleted the jeremypw/gtk4-prep/BookmarkListBox-cleanup branch March 8, 2025 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants