- Регистрация
- 1 Мар 2015
- Сообщения
- 1,816
- Баллы
- 155
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 -
New Releases
A new version of was released recently, and it includes my new feature! You know what else was released? ! 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 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 , 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 method, which does just that!
I wrote a 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 -
Turned out a lot of people don't understand SQL
After I created this PR, the reviewer . 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 , 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 .
Turned out a lot of people, even experienced developers, don't understand SQL!
Mattermost Mobile -
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 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 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 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.
Tiling Shell -
New Releases
A new version of was released recently, and it includes my new feature! You know what else was released? ! 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 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 , 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 method, which does just that!
I wrote a 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 -
Turned out a lot of people don't understand SQL
After I created this PR, the reviewer . 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 , 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 .
Turned out a lot of people, even experienced developers, don't understand SQL!
Mattermost Mobile -
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 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 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 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.