Hacktoberfest! - Week 4

Last week of October. Turned out things just couldn't stop coming back! I got some more interesting activities on all 3 PRs I've recently worked on

Tiling Shell - domferr/tilingshell#151

New Releases

A new version of tiling shell was released recently, and it includes my new feature! You know what else was released? Fedora 41! With the brand new GNOME 47. It's such a wonderful feeling to see my hard work working perfectly™️ on the freshly updated system!

Feedback from a User

As soon as the new version release, someone reached out to us and gave us some feedback on the PR.

Although not necessarily a bug, they pointed that the file chooser lacked a clear indicator of what file extension should be used. Here's what it looks like right now:

This was something I noticed during development as well. I had tried to apply a filter, which was what the author did with the "Export layout" button. However, it seemed to only filter on existing files, not the one that's about to be created.

Later, the author made a change to remove the filter, so I followed his lead and didn't question it that much.

However, this user brought up the idea to pre-fill the file name. It was such a simple solution but none of us had thought of it. I did a quick search and found the Gtk.FileChooser.set_current_name method, which does just that!

I wrote a comment to suggest this fix to the author. He quickly confirmed it and stated that he would include this in the next release. Hooray!

Mattermost Mobile - mattermost/mattermost-mobile#8278

Turned out a lot of people don't understand SQL

After I created this PR, the reviewer recommended a change. They suggested adding a UNIQUE keyword to the CREATE INDEX query, so that duplicate indices can't exist. Sounds reasonable, right? Without much understanding of SQL, I blindly applied the change, trusting them to know what they were doing.

Later, another maintainer made a follow-up comment, suggesting that it might not be appropriate here, but they "may be misunderstanding this".

A few days later, when another reviewer came in and tested the code, they finally realized that the migration failed. - Turned out the UNIQUE keyword doesn't at all do the thing they thought it would do: It made sure the inserted values in this row are unique, not the index itself. And it was cleared and concisely explained in the SQLite doc.

Turned out a lot of people, even experienced developers, don't understand SQL!

Mattermost Mobile - mattermost/mattermost-mobile#8260

With the other PR merged, this one should be ready to merge as well. At least that's what I thought.

After a second review, the reviewer made a comment about the post deletion process.

He pointed that when a pull-to-refresh action is done, deletion was performed among all posts, while it should be confined within the current channel/thread.

This was mentioned in the original issue as well. To be perfectly honest, I missed that part as well.

I went back to the code, and started working on the fix straight away. However, when I finally pushed the code, the reviewer reversed it soon after.

I was very confused, until I found the comment made by him. He explained that this change was, after all, not needed, since the posts are passed as props, and only the posts in the current screen are passed down.

If I had done more testing before, We wouldn't need to go through this at all.

Conclusion

The overall theme of this week has been small changes turning into trouble, which turned out to be not necessary at all! I think the lesson here is to always test your code before and after you work on any feedback, no matter how trivial they look.