Gajim - 2023-07-23

``` def get_last_message_row(self) -> MessageRow | None: children = self._list_box.get_children() children.reverse() ```

what is the role of `reverse()`here?

depends of the rest of the code

reverse reverses a list

is there room for optimization for `get_last_message()` method in view.py?

I was wondering how costly it is to reverse the whole list to get the last item as the 0'th item.

I am aware that the number of rows in the `Listbox` of `ConversationView` is limited to 100.

But still...

Sorry the method I am talking about is `get_last_message_row`; missed the 'row' in the previous message.

but we dont want always the last item in the list

we only want it if its a message row

So `reverse` helps so that we have to check fewer rows?

no, we want to iterate the list in revered order

of course you can do this in many ways in python

but its not simply accessing the last item, that would be trivial list[-1]

you can ask the question what is the fastest way in python to traverse a list in reversed order

Yup, I get it now. There could be read markers, date rows as the last item in the list.

i cant give you the answer, there might be faster ways then to reverse the list, but this can only found out by timing various methods

as its not evident, what the C implementation of python actually does with the various approaches

kurion, the first rule of performances optimisations is: profile.

If your function doesn’t even appear on the profile, don’t bother optimising it.

If you find out that your function is indeed slower than it could be, profile again at every step you take to optimise it.

That way you will know which approaches improve things, which don’t do anything, and which actually decrease performances.

It’s rarely intuitive, unless you have a very deep understanding of all of the layers, from the CPU architecture to the Python internals, so always profile.

i think his first thought was, we want indeed only the last item of the list, in this case the assumption that reversing the list is inefficient would be correct

Sure, but if this is a function called only once in a while optimising it doesn’t matter.

Link: Agreed. Premature optimization is a hazard.

In this particular case though, assuming a large list and you only want to iterate on the last few items, the reversed() iterator would likely be a performance win.

It iterates from the end of the list, but doesn’t actually reverse it.

Yes

and the motto about premature optimization, does not mean we should ignore obvious inefficent code

just because it does not yet show up on some profile

Of course.

for me it just means we should not put in a disproportionate amount of time into optimizing something that may not even have a visible impact

I am really happy that trying to make modifications to a FOSS project is making me learn new thoughts and concepts from you guys. ✏

Actually, I was trying to make Gajim as keyboard operable as possible for my blind friend (and for me too). So I made the shortcut Ctrl+L to move focus to the last selected/visited MessageRow from anywhere in the ChatPage. When no message is visited at all, focus goes to the last message. So that method will be used a considerable number of time in my use case.

css-parser seems to be the slowest part of startup, with a tiny account.

then use `for row in reversed(children):` its probably the fastest way

kurion, I also happen to have a blind friend, this might help make her move away from Pidgin. :)

Since Link is active at the moment and he was interested in keyboard operability, I'd like to say that I have made some hotkeys for `MessageRow's and the `listbox` like copying multiple selected messages, quoting a message etc. The UX feels great already

Did you start merging some of these features back to Gajim btw?

no, only made changes locally so far

some of the implementation contains code smell like using methods with underscore prefix outside of a class.

so I am afraid that some of the changes are not _pull-request-worthy_.

hey lovetox! are PMs here disabled?

yes, but dont seem to work

oh OK. can you add me xmpp:nicoco@nicoco.fr maybe?

I'd like to have the 'Add Account' MenuItem listed in the Accounts ribbon menu at all times.

So I removed the conditional in the `build_accounts_menu` method in accounts.py

hm this was the case anyway

its just named differently

Modify Accounts

yes just realized

its always named Modify accounts, except there is no account configured at all

and in this case the account wizard is shown

so its very edge case that any normal user would ever see the "Add Account" Menu

I replaced `'app.accounts::` in ``` add_account_item = Gio.MenuItem.new(_('_Add Account…'), 'app.accounts::') ``` with `'app.add-accounts::' ✏

And it wasn't working.

because that action odes not exist ..

then I changed ```('add-account', None),``` in const.py to ```('add-account', 's'),```

and it was okay.

sorry I put an extra 's' at the end of my previous message

But I wonder what made it work when I changed None to 's'.

I just followed the pattern of 'add-contact' and saw that it had an 's'.

What actually changed here?

kurion, the 's' means that action takes a string as an argument.

It’s like when you define a function, it can have no argument (e.g. foo()) or one string argument (e.g. bar("quux")).

Now I see why add-contact needs a string argument. It tells the function which account to add the contact to, right?

and :: means empty string

_on_add_contact_action

then you will see what is done with the argument

the argument is the jid of the contact

and if its supplied we can skip the page where the user needs to input the jid, in the add-contact dialog

Oh, what a hit and a miss! :P

Still couldn't figure out why it didn't work when add-account had None as the argument in const.py? ✏

because the action passes a empty string

:: means empty string

Oh, so if I remove the :: , it should go along well with None, right?

yes, until it hits the callback

which probably also exepects one argument

I see that in 'applications.py', `_on_add_account_action` does not require an argument. ✏

It's None, acually

Thanks for the help. It is working now

Has https://dev.gajim.org/gajim/gajim-plugins/-/milestones/2#tab-issues entered a release? None is stated at the site.