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

Fix blank room screen #6565

Merged
merged 6 commits into from
Aug 17, 2022
Merged

Fix blank room screen #6565

merged 6 commits into from
Aug 17, 2022

Conversation

ismailgulek
Copy link
Contributor

@ismailgulek ismailgulek commented Aug 12, 2022

I couldn't reproduce the issue by playing with network connection. I could see a blank screen but also a syncing spinner was there.

The issue seen on iPad was due to an unnecessary inputAccessoryView on message text view. Somehow the table view was insetted wrongly (so that all the content to be out of screen).
This PR also addresses some suspicious UITableView errors i saw in logs.
There may be other cases for the issue we're not aware of yet.

  1. Element[4045:46257] [Assert] Attempted to access the table view's visibleCells while they were in the process of being updated, which is not allowed. Make a symbolic breakpoint at UITableViewAlertForVisibleCellsAccessDuringUpdate to catch this in the debugger and see what caused this to occur. Perhaps you are trying to ask the table view for the visible cells from inside a table view callback about a specific row? Table view: <UITableView: 0x7fc1d0150000; frame = (0 0; 428 777); autoresize = RM+BM; gestureRecognizers = <NSArray: 0x6000010835a0>; layer = <CALayer: 0x600001d853c0>; contentOffset: {0, 0}; contentSize: {428, 1998.6666641235352}; adjustedContentInset: {0, 0, 34, 0}; dataSource: <RoomDataSource: 0x7fc1c72d13e0>>

  2. Element[4288:55918] [TableView] Warning once only: UITableView was told to layout its visible cells and other contents without being in the view hierarchy (the table view or one of its superviews has not been added to a window). This may cause bugs by forcing views inside the table view to load and perform layout without accurate information (e.g. table view bounds, trait collection, layout margins, safe area insets, etc), and will also cause unnecessary performance overhead due to extra layout passes. Make a symbolic breakpoint at UITableViewAlertForLayoutOutsideViewHierarchy to catch this in the debugger and see what caused this to occur, so you can avoid this action altogether if possible, or defer it until the table view has been added to a window. Table view: <UITableView: 0x7fc927027a00; frame = (0 0; 428 868); hidden = YES; autoresize = RM+BM; gestureRecognizers = <NSArray: 0x6000018f4360>; layer = <CALayer: 0x6000016d5060>; contentOffset: {0, 0}; contentSize: {428, 0}; adjustedContentInset: {0, 0, 0, 0}; dataSource: (null)>

  3. iPad issue after orientation changes (reported at https://matrix.to/#/!NMoyFpQxIVYaQIfQHG:matrix.org/$Pf9_zjq5XrNPmWCI57DjdYnFcffH3NcS0iNHJKvx6rY?via=matrix.org&via=element.io&via=one.ems.host)

Fixes #5932

Copy link
Member

@pixlwave pixlwave left a comment

Choose a reason for hiding this comment

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

Nice catches 👏
Overall seems good to me, but one thought inline.

Comment on lines -83 to -86

// Add an accessory view to the text view in order to retrieve keyboard view.
inputAccessoryView = [[UIView alloc] initWithFrame:CGRectZero];
self.textView.inputAccessoryView = inputAccessoryView;
Copy link
Member

@pixlwave pixlwave Aug 12, 2022

Choose a reason for hiding this comment

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

Have you checked against #5042 to see if this change causes a regression on iPhone?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it caused, i've reverted that (including a name change to avoid confusion). I've fixed the issue 3 by updating the constraints of table view in the room screen.

@github-actions
Copy link

github-actions bot commented Aug 12, 2022

📱 Scan the QR code below to install the build for this PR.
🔒 This build is for internal testing purpose. Only devices listed in the ad-hoc provisioning profile can install Element Alpha.

QR code

If you can't scan the QR code you can install the build via this link: https://i.diawi.com/ceBgea

@codecov-commenter
Copy link

Codecov Report

Merging #6565 (98338fd) into develop (6f0d796) will increase coverage by 0.01%.
The diff coverage is 0.00%.

@@             Coverage Diff             @@
##           develop    #6565      +/-   ##
===========================================
+ Coverage     6.28%    6.29%   +0.01%     
===========================================
  Files         1458     1458              
  Lines       153924   153931       +7     
  Branches     61867    61875       +8     
===========================================
+ Hits          9668     9692      +24     
+ Misses      143850   143831      -19     
- Partials       406      408       +2     
Impacted Files Coverage Δ
Riot/Modules/Room/MXKRoomViewController.m 0.00% <0.00%> (ø)
Riot/Modules/Room/RoomViewController.m 0.00% <0.00%> (ø)
...les/Room/Views/InputToolbar/RoomInputToolbarView.m 0.00% <ø> (ø)
...ing/SplashScreen/View/OnboardingSplashScreen.swift 58.66% <0.00%> (+4.00%) ⬆️
...Modules/Common/ViewModel/StateStoreViewModel.swift 50.00% <0.00%> (+19.44%) ⬆️
...SplashScreen/OnboardingSplashScreenViewModel.swift 53.84% <0.00%> (+42.30%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@sonarcloud
Copy link

sonarcloud bot commented Aug 15, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

Copy link
Member

@pixlwave pixlwave left a comment

Choose a reason for hiding this comment

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

LGTM, updated naming of the accessory view makes much more sense now too :)

@ismailgulek ismailgulek merged commit adfe56b into develop Aug 17, 2022
@ismailgulek ismailgulek deleted the ismail/5932_blank_room_screen branch August 17, 2022 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Blank room screen when resuming the app
3 participants