Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve window title behaviour #1881

Closed
wants to merge 0 commits into from
Closed

Improve window title behaviour #1881

wants to merge 0 commits into from

Conversation

ike08
Copy link
Contributor

@ike08 ike08 commented Aug 27, 2023

Cleanup the window title code by adding a function to mucwin.c that properly sets the window title. Remove window title code from iq.c and message.c and add calls to the new window title function. Below are three goals accomplished.

New feature: Display bookmark name as the window title.

Bug Fix 1: Profanity segfaults if a Room Name is NULL after re-retrieving the MUC's Disco information. Eg, after using "/room config" and "/form submit" when Room Name is blank.

Bug fix 2: MUC windows show an empty window title when joining a room that has a NULL Room Name.

HOW TO TEST message.c

  1. Create a muc and set Room Name to "RoomName".
  2. Create a bookmark for that room and set the bookmark Name to "BMRoomName".
  3. Configure your room and submit the configuration without making changes.
  4. The window title should show your bookmark Name, "BMRoomName".
  5. Delete and recreate your bookmark, but do not set a Name.
  6. Configure your room and submit the configuration without making changes.
  7. The window title should be your Room Name, "RoomName".
  8. Delete your bookmark.
  9. Configure your room and submit the configuration without making changes.
  10. The window title should be your Room Name, "RoomName".
  11. Configure your room, set Room Name to "", and submit your change.
  12. The window title should be the room JID.

HOW TO TEST iq.c

  1. Create a muc and set Room Name to "RoomName".
  2. Create a bookmark for that room and set the bookmark Name to "BMRoomName".
  3. Leave and rejoin your room.
  4. The window title should be your bookmark Name, "BMRoomName".
  5. Delete and recreate your bookmark, but do not set a Name.
  6. Leave and rejoin your room.
  7. The window title should show your Room Name, "RoomName".
  8. Delete your bookmark.
  9. Leave and rejoin your room.
  10. The window title should show your Room Name, "RoomName".
  11. Configure your room to set Room Name to "".
  12. Leave and rejoin your room.
  13. The window title should be the room JID.

I ran valgrind.

@jubalh jubalh self-assigned this Aug 28, 2023
@jubalh jubalh added this to the next milestone Aug 28, 2023
@jubalh
Copy link
Member

jubalh commented Aug 28, 2023

Thanks that looks good!

I only think we might need to describe this somewhere to the user. What will be used as the MUC name.
What do you think about that?

Specifically /roster room use name comes to mind. Where the description so far is: Use the MUC name as room name..
Maybe there are other locations too.

@ike08
Copy link
Contributor Author

ike08 commented Aug 29, 2023

Good point. It looks like /roster and /statusbar let the user choose between displaying the room name or jid.

/roster room use name|jid
/statusbar room room|jid

Maybe /titlebar should behave the same way. What do you think about replacing "/titlebar show|hide name|jid" with "/titlebar title bookmark|name|jid"? The user will not be able to hide the window title anymore, but /titlebar will behave like /roster and /statusbar instead.

Bookmark name should be an option in /roster and /statusbar too.

@jubalh
Copy link
Member

jubalh commented Aug 29, 2023

That sounds good.

@jubalh jubalh self-requested a review August 29, 2023 14:53
Copy link
Member

@jubalh jubalh left a comment

Choose a reason for hiding this comment

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

As mentioned in the comments. Just using this so that I know that I reviewed this PR and don't check it again until required :)

src/ui/mucwin.c Outdated
mucwin_set_title(const char* const room_jid, const char* const disco_name)
{
ProfMucWin* win = wins_get_muc(room_jid);
if (win) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe just write error in log and return FALSE if !win? It would be more flexible approach

@jubalh
Copy link
Member

jubalh commented Sep 11, 2023

@ike08 ping :)

@ike08 ike08 closed this Sep 21, 2023
@jubalh
Copy link
Member

jubalh commented Sep 22, 2023

@ike08 could you post here a small summary why you closed the PR? For future reference :)

@ike08
Copy link
Contributor Author

ike08 commented Sep 22, 2023

I needed to sync my fork to master before committing new changes. I didn't know GitHub would close my PR when I clicked the sync button. I will reference this PR in my next PR.

@jubalh
Copy link
Member

jubalh commented Sep 22, 2023

I see! It's best to always work on a branch other than master.

@ike08
Copy link
Contributor Author

ike08 commented Sep 22, 2023

Can you explain why? I'm not trying to be contentious; I'm new to Git and it is not obvious to me. My new code will need to be merged, so it makes sense to add the new code to the most recent version of master to reduce merging conflicts.

@jubalh
Copy link
Member

jubalh commented Sep 22, 2023

Sure, I'll try :)

Usually I do:
git clone https://github.com/profanity-im/profanity
then add my remote git remote add mine https://github.com/jubalh/profanity

Now I go to work on git checkout -b feature/my-cool-feature.

In the meantime jaeckel codes on his cool feature. Creates a PR. It gets merged upstream.
And I realize I want to have his changes in my feature branch as well because it changes something important for me.

So now I can easily do git checkout master and git pull to have the latest master locally.
I can git checkout feature/my-cool-feature and git rebase maste to rebase it on master. I could merge the master commits.. I could create a new branch ontop of master.... I have all the options.

If I code on master directly then I will have merge conflicts and diverging branches locally vs upstream.
If several people work on a problem it will be a mess if one works on master.

@ike08
Copy link
Contributor Author

ike08 commented Sep 22, 2023

Brilliant. That is much better than what I was trying to do. Thank you

@H3rnand3zzz
Copy link
Contributor

H3rnand3zzz commented Sep 25, 2023

So now I can easily do git checkout master and git pull to have the latest master locally. I can git checkout feature/my-cool-feature and git rebase maste to rebase it on master. I could merge the master commits.. I could create a new branch ontop of master.... I have all the options.

You can use git checkout -, it will move you to the previous branch. The same style as a cd -.

E.g. we are on the feature branch and we want to merge our branch with master, then we do

git checkout master # getting to the master branch so we can pull updates for it
git pull # pulling the updates that Jaeckel pushed, hence updating our local master branch
git checkout - # now we get back to our feature branch
git rebase master # now we are rebasing on the master branch, so we can get our branch updated locally
git push --force-with-lease # pushing the update in a somewhat safe manner to update its remote part (on Github in this case)

And if you have some changes that you haven't yet applied (and don't want to apply yet), you can use git stash before the described actions and git unstash after so the local changes will be "stashed" away.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants