Add conversation list as sidebar in Talk UI for iPads and Macs

[claucambra]

This PR adds a split view to the Talk UI, improving its usability on the iPad and on macOS.

Stuff still to be done:

• [x] Ensure table view cell for selected room is highlighted
• [x] Add intro screen to secondary view when no chats are open (e.g. "Start a conversation" or something along those lines)
• [x] Fix borking of toolbars caused during collapse/reopen of primary view (the sidebar)
• [x] Fix centering of unread mentions toast
• [x] Fix the colour of the sidebar toggle button in the primary view (almost invisible with the default blue)
• [x] Fix incorrect disabling of chat controls
• [x] Fix incorrect highlighting of chats
• [x] Fix switching accounts, leaving conversations, removing accounts laving chat open in secondary view

Closes #302

[claucambra]

Should be ready for review now :)

[claucambra]

Just noticed the room highlight seems to stick to position instead of to the room it is supposed to be highlighting. Will fix

[claucambra]

Tried it running on my Mac (Designed for iPad). Noticed that when selecting a different conversation (after sending a msg) it disables the message bar / textfield.

I've looked at it and I am getting some very weird behaviour with the text field too, will take a closer look

[claucambra]

Tried it running on my Mac (Designed for iPad). Noticed that when selecting a different conversation (after sending a msg) it disables the message bar / textfield.

Took me way too long to figure out but it seems the new chat room was picking up on the notification to disable the old chat room's controls after leaving -- fixed now

[claucambra]

Okay, fixed the major issues we found during the last round of reviews, hopefully we don't find any more ;) @Ivansss @thisIsTheFoxe

[claucambra]

Okay, fixed the major issues we found during the last round of reviews, hopefully we don't find any more ;)

Well, I'm sorry to disappoint you.. 😅 If this is out of scope for just adding the sidebar or you want to fix them in a separate PR let me kno..

There are a few more things I noticed (again, on Mac designed for iPad):

• Switching accounts keeps the current chat view open

  • it even still allows you to send chat messages with the other account that I assume is still in memory, even tho I changed the active user

• Similarly, leaving a conversation keeps it open

• Similarly, removing the account entirely keeps the chat view open

  • Interestingly, sent messages don't appear anymore but are still sent to the server :D
  • The view is also still active after re-logging-in (e.g. with a different user)

Other than that:

• the chat room Lobby toolbar can get stuck when selecting a date
• Setting the status text requires you to press enter, to enable the button, similarly clearing everything doesn't disable the button right away (isn't noticed, cuz 'normally' iOS keyboard covers the button)

Oof. 😳

Big thanks @thisIsTheFoxe for the thorough review -- not disappointed, more shocked!

I will get started fixing these issues soon, seems like there are still issues with room-leaving procedures... No need for a separate PR, IMO these issues are deal-breakers and should be fixed in this PR

[claucambra]

Okay @thisIsTheFoxe hopefully third time's the charm 😄

I think I've fixed all the weirdness with chats remaining open, broken highlights, and so on. Hopefully you won't find any other broken stuff around the split view!

Maybe we should take care of the lobby start time and status message in another PR? This one is getting pretty big already and these two issues are a bit out of scope, I think

[SystemKeeper]

Hey @claucambra I noticed that rooms are not always left correctly, resulting in all messages send by other users marked as read immediately. This PR when switching from one conversation to another:

2022-05-26 19:55:17.943213+0200 NextcloudTalk[17961:247552] Starting chat in room: test2
2022-05-26 19:55:17.943752+0200 NextcloudTalk[17961:247552] Creating new chat view controller.
2022-05-26 19:55:18.082703+0200 NextcloudTalk[17961:247552] Sending: {"type":"room","room":{"roomid":"h6m9r34j","sessionid":""}}
2022-05-26 19:55:18.136645+0200 NextcloudTalk[17961:249007] WebSocket didReceiveMessage: {"type":"room","room":{"roomid":"h6m9r34j","properties":{"name":"test2","type":1,"lobby-state":0,"lobby-timer":null,"read-only":0,"listable":0,"active-since":null,"sip-enabled":0,"description":""}}}
2022-05-26 19:55:18.137115+0200 NextcloudTalk[17961:249007] WebSocket didReceiveMessage: {"type":"event","event":{"target":"room","type":"join","join":[{"sessionid":"","userid":"test","user":{"displayname":"test"}}]}}
2022-05-26 19:55:18.137162+0200 NextcloudTalk[17961:249007] App user joined room.

Master when switching from one conversation to another:

2022-05-26 20:06:37.300915+0200 NextcloudTalk[20255:266508] Sending: {"type":"room","room":{"roomid":"","sessionid":""}}
2022-05-26 20:06:37.303845+0200 NextcloudTalk[20255:266778] WebSocket didReceiveMessage: {"type":"room","room":{"roomid":""}}
2022-05-26 20:06:37.339952+0200 NextcloudTalk[20255:266508] Rooms updated
2022-05-26 20:06:47.499907+0200 NextcloudTalk[20255:266508] Sending: {"type":"room","room":{"roomid":"h6m9r34j","sessionid":""}}
2022-05-26 20:06:47.553548+0200 NextcloudTalk[20255:267605] WebSocket didReceiveMessage: {"type":"room","room":{"roomid":"h6m9r34j","properties":{"name":"test2","type":1,"lobby-state":0,"lobby-timer":null,"read-only":0,"listable":0,"active-since":null,"sip-enabled":0,"description":""}}}
2022-05-26 20:06:47.553715+0200 NextcloudTalk[20255:267605] WebSocket didReceiveMessage: {"type":"event","event":{"target":"room","type":"join","join":[{"sessionid":"","userid":"test","user":{"displayname":"test"}}]}}
2022-05-26 20:06:47.553807+0200 NextcloudTalk[20255:267605] App user joined room.

Notice the first websocket exchange, which is missing using this PR (this Test is done with HPB enabled). I guess the ChatViewController is still active and receiving messages, although not visible anymore.

I don't think this file should be added: /project.xcworkspace/xcuserdata/claucambra.xcuserdatad/UserInterfaceState.xcuserstate

I haven't tested, but we need to make sure, that the strings you added in the storyboard are correctly added to the Localizable.strings file.

We should discuss if the changes to the pod file are actually neccessary, master sets the target in the second line of the file (see https://github.com/nextcloud/talk-ios/commit/2e88af7e7a2ab8ebc818db88198ace29dbe395bd#diff-8f7d6adf31268a2d897ee34bd170592648d6e520aa237104395e4a4438af50cb for example) and does not set EXCLUDED_ARCHS (same for project file)

[claucambra]

Hey @SystemKeeper, thanks for your comments -- regarding your points:

1. I removed the user-specific stuff from the project.xcworkspace folder
2. After testing it's clear that the additions to the Podfile weren't needed so I removed them
3. I added Main.storyboard to Xcode's Localisation which generated Main.strings for each language -- hopefully this isn't a problem :)

I will take a look at the rooms not being left correctly now, thanks again!

[claucambra]

@SystemKeeper rooms are now left correctly -- new messages from rooms you were previously in are now correctly marked as unread

[SystemKeeper]

I'm sorry @claucambra that's not what I'm seeing here (again HPB enabled).

When running the app on M1 macs, NCRoomsManager leaveRoom is indeed called when switching rooms: https://github.com/nextcloud/talk-ios/blob/645fde1a159396cdb2f1e3ab56936ae144cc204e/NextcloudTalk/NCRoomsManager.m#L224

By the time we arrive at completion block of the exitRoom API call, we're already joined in the new room, therefore never sending a leave to the HPB: https://github.com/nextcloud/talk-ios/blob/645fde1a159396cdb2f1e3ab56936ae144cc204e/NextcloudTalk/NCExternalSignalingController.m#L284-L290 (It might be a good idea to have a look at this in general. I can imagine that this can happen also for slow servers/connections?!)

I see a general problem here: With the new splitview you're basically always joined in a room, which means you're unable to leave completely without closing the app or switching to another account. Because of this, your session in this room is active in the background, so you're still receiving messages, marking them as read and prevent push notifications to be send. Looks like appDidBecomeActive and appWillResignActive will only be called when the app is minimized. Maybe you can confirm this? One way would be to conditionally listen to NSApplicationDidResignActiveNotification and NSApplicationDidBecomeActiveNotification when running on M1 Mac. These will be called when the app looses focus in a quick test. I'm no expert, so there might be a better way of handling this... Let's see if @Ivansss has some input on this.

It's even worse when running on an iPhone, in this case leaveRoom never gets called, essentially leaving your session in this room active and still receiving messages. When I've been previously in a room, there's no join event either, because there's always a RoomController around: Failing this check: https://github.com/nextcloud/talk-ios/blob/645fde1a159396cdb2f1e3ab56936ae144cc204e/NextcloudTalk/NCRoomsManager.m#L109-L114 This is not the case with current master.

Also please check the statusbar in your PR (top picture is master)

Regarding the translation: I think this will not work with the current configuration. Most of the time translation is done in code for interfaces, like here https://github.com/nextcloud/talk-ios/blob/645fde1a159396cdb2f1e3ab56936ae144cc204e/NextcloudTalk/NewRoomTableViewController.m#L177 But before you change that again, let's wait for Ivans input 😄

[claucambra]

I'm sorry @claucambra that's not what I'm seeing here (again HPB enabled).

When running the app on M1 macs, NCRoomsManager leaveRoom is indeed called when switching rooms:

https://github.com/nextcloud/talk-ios/blob/645fde1a159396cdb2f1e3ab56936ae144cc204e/NextcloudTalk/NCRoomsManager.m#L224

By the time we arrive at completion block of the exitRoom API call, we're already joined in the new room, therefore never sending a leave to the HPB:

https://github.com/nextcloud/talk-ios/blob/645fde1a159396cdb2f1e3ab56936ae144cc204e/NextcloudTalk/NCExternalSignalingController.m#L284-L290

(It might be a good idea to have a look at this in general. I can imagine that this can happen also for slow servers/connections?!)

You're right that the exitRoom completion handler does seem problematic. This part of the code hasn't been changed in this PR and as far as I can tell, it is a possible issue also present in master. I will leave it up to @Ivansss to decide if this should be fixed in this PR or if it should be separate 🙂

I see a general problem here: With the new splitview you're basically always joined in a room, which means you're unable to leave completely without closing the app or switching to another account. Because of this, your session in this room is active in the background, so you're still receiving messages, marking them as read and prevent push notifications to be send. Looks like appDidBecomeActive and appWillResignActive will only be called when the app is minimized. Maybe you can confirm this? One way would be to conditionally listen to NSApplicationDidResignActiveNotification and NSApplicationDidBecomeActiveNotification when running on M1 Mac. These will be called when the app looses focus in a quick test. I'm no expert, so there might be a better way of handling this... Let's see if @Ivansss has some input on this.

The NCChatViewController components already listen to appDidBecomeActive and appWillResignActive and handle stopping/resuming the chat accordingly -- in my testing it seems that iOS applications on macOS only "resign active" when they are in a separate workspace or minimised, not when they lose focus.

It's even worse when running on an iPhone, in this case leaveRoom never gets called, essentially leaving your session in this room active and still receiving messages.

Fixed this in a new commit, thanks for pointing it out!

When I've been previously in a room, there's no join event either, because there's always a RoomController around: Failing this check:

https://github.com/nextcloud/talk-ios/blob/645fde1a159396cdb2f1e3ab56936ae144cc204e/NextcloudTalk/NCRoomsManager.m#L109-L114

This is not the case with current master.

Also please check the statusbar in your PR (top picture is master)

Thanks for pointing this out too, I had totally missed it. Also fixed in a new commit.

Regarding the translation: I think this will not work with the current configuration. Most of the time translation is done in code for interfaces, like here

https://github.com/nextcloud/talk-ios/blob/645fde1a159396cdb2f1e3ab56936ae144cc204e/NextcloudTalk/NewRoomTableViewController.m#L177

But before you change that again, let's wait for Ivans input 😄

I agree, I don't know anything about the translation workflow!

Thanks again for your comments :)

[SystemKeeper]

You're right that the exitRoom completion handler does seem problematic. This part of the code hasn't been changed in this PR and as far as I can tell, it is a possible issue also present in master.

Currently, with master, when you go to the conversation list, the ViewController is dismissed, therefore correctly leaving a room. When you select another room, this room will be joined. Now with splitView leaving and joining is essentially the same operation, which makes it prone to this race condition. Also the controllers will not be released correctly.

The NCChatViewController components already listen to appDidBecomeActive and appWillResignActive and handle stopping/resuming the chat accordingly -- in my testing it seems that iOS applications on macOS only "resign active" when they are in a separate workspace or minimised, not when they lose focus.

Exactly. Notice that the app is currently listening to UIApplicationDidBecomeActiveNotification not NSApplicationDidBecomeActiveNotification which is only available on mac and in my tests will be called whenevery the app looses its focus. So that would be a workaround for this problem. Still the question if a users should be able to activley leave a room.

[SystemKeeper]

Fixed this in a new commit, thanks for pointing it out!

Haven't tested yet, but now you're calling stopChat which will not leave the chat, just prevent new messages from arriving. IIRC this will prevent push notifications, because you still have an active session. Is there reason you wanna keep the controllers around?

[claucambra]

Fixed this in a new commit, thanks for pointing it out!

Haven't tested yet, but now you're calling stopChat which will not leave the chat, just prevent new messages from arriving. IIRC this will prevent push notifications, because you still have an active session. Is there reason you wanna keep the controllers around?

Okay, understood -- I changed this for a call to the room manager's leaveRoom which should kill the old chat controller when it disappears. No need to keep it around

[claucambra]

Ping? :)

[Ivansss]

Thank you so much for the PR @claucambra :) I will take a look at it soon :D

[SystemKeeper]

Well... that was, uhm, interesting 😅

Okay, so I've updated the branch with changes discussed with Ivan. We only support split view on iOS 14 and above, all other devices will use the current implementation

There were still quite a few things to solve, the main problem was changing from collapsed to expanded and vise versa. An iPad mini for example usually uses split view, so room list is always visible, but when you enter the iOS split view (so 2 apps beside each other) it will show collapsed mode. That needs to be addressed. Also we needed to make sure that the NCChatViewController is actually dealloced (UISplitViewController holds 2 references, in case you want to show the detailed view again).

Testcases:

Test cases splitViewController

Test 1 - Compact mode
- Start in compact mode (like iPhone)
- Enter Room
- Leave Room
-> Check if we see RoomListTableViewController
-> Check if NCChatViewController is correctly deallocated

Test 2 - Expanded mode
- Start in expanded mode (like iPad)
- Enter Room
- Leave Room
-> Check if we see PlaceholderViewController
-> Check if NCChatViewController ist correctly deallocated

Test 3 - Compact to Expanded 1
- Start in compact mode (like iPhone)
- Enter Room
- Leave Room
- Change to expanded mode (like going from portrait to landscape on iPhone 11 Max)
-> Check if we see the PlaceholderViewController
-> Check if NCChatViewController is correctly deallocated

Test 4 - Compact to Expanded 2
- Start in compact mode (like iPhone)
- Enter room
- Change to expanded mode (like going from portrait to landscape on iPhone 11 Max)
-> Check that you still see the conversation
-> Check that when you leave the conversation, you see the PlaceholderViewController

Test 5 - Expanded to Compact 1
- Start in expanded mode (like iPad)
- Enter room
- Leave room
- Change to compact mode (like going from landscape to portrait on iPhone 11 Max)
-> Check that you see RoomListTableViewController
-> Check that NCChatViewController is correctly deallocated

Test 6 - Expanded to Compact 2
- Start in expanded mode (like iPad)
- Enter room
- Change to compact mode (like going from landscape to portrait on iPhone 11 Max)
-> Check that you still see the conversation
-> Check that when you leave the conversation, you see the RoomListTableViewController

Notes:

• iOS 13 crashes when trying to make a call, but that's also the case for master, so I suspect something wrong in the simulator (it worked on iOS 16)

Known Issues:

• [x] RoomList is not updated when a new room is created (and in case there's a open chat, it's not remove properly)
• [x] You can delete a room from room list while still being in that chat
• [x] Room is not left, when switching accounts
• [x] Color of back arrow is wrong after switching accounts
• [x] Color of placeholderView is wrong after switching accounts

Besides the known issues, I would love to get some feedback 😉 @claucambra @Ivansss

[claucambra]

Well... that was, uhm, interesting sweat_smile

Bitrot is a real thing :)

Okay, so I've updated the branch with changes discussed with Ivan. We only support split view on iOS 14 and above, all other devices will use the current implementation

That seems like a good idea IMO

There were still quite a few things to solve, the main problem was changing from collapsed to expanded and vise versa. An iPad mini for example usually uses split view, so room list is always visible, but when you enter the iOS split view (so 2 apps beside each other) it will show collapsed mode. That needs to be addressed. Also we needed to make sure that the NCChatViewController is actually dealloced (UISplitViewController holds 2 references, in case you want to show the detailed view again).

I am not sure I understand the issue here? What is the problem that happens when the view is collapsed? The room view should be shown on collapse instead of the rooms list view?

Besides the known issues, I would love to get some feedback wink @claucambra @Ivansss

Please let me know if there is something I can do to help push this forward :)

[SystemKeeper]

I am not sure I understand the issue here? What is the problem that happens when the view is collapsed? The room view should be shown on collapse instead of the rooms list view?

So when you are in expanded mode (like iPad), you have a primary and a secondary column. When you enter a conversation, the chatViewController gets added to the secondary column. Now if you rotate the device in that way, that you switch to collapsed mode, you still see the conversation -> fine. If the conversation is then left the chatViewController should be deallocated, but that doesn't happen, because if you (after returning to the room list in collapsed mode) switch back to expanded mode, the supposedly deallocated chatViewController was back again. Bottom line: Whenever a conversation is left (be it collapsed or expanded) the chatViewController should be deallocated and not in memory anymore.

Edit: To be clear, that should work now :)

Please let me know if there is something I can do to help push this forward :)

With the latest commits done today all known issues should be fixed (known issues in "issues I found while testing" 😅). It would be great if you could give this branch a try and report back if you find something odd :-) (or not, if everything works 😉 )

[SystemKeeper]

Fixed and rebased 😄

[claucambra]

@SystemKeeper thank you for making the PR mergeable!!