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

0.5 broke something pretty fundamental #22

Closed
sjmcdowall opened this issue Jan 8, 2019 · 22 comments
Closed

0.5 broke something pretty fundamental #22

sjmcdowall opened this issue Jan 8, 2019 · 22 comments

Comments

@sjmcdowall
Copy link
Collaborator

On 0.4.X this work working just fine -- I upgraded to 0.5.0 and now for the same Widgets I am getting the below stack trace (and red screen of death on the phone).

Any ideas?

══╡ EXCEPTION CAUGHT BY WIDGETS LIBRARY ╞═══════════════════════════════════════════════════════════
flutter: The following assertion was thrown building _SuggestionsList<dynamic>(dirty, state:
flutter: _SuggestionsListState<dynamic>#feb23(ticker inactive)):
flutter: BoxConstraints has non-normalized height constraints.
flutter: The offending constraints were:
flutter:   BoxConstraints(w=Infinity, 358.4<=h<=300.0; NOT NORMALIZED)
flutter:
flutter: When the exception was thrown, this was the stack:
flutter: #0      BoxConstraints.debugAssertIsValid.<anonymous closure>.throwError (package:flutter/src/rendering/box.dart:504:9)
flutter: #1      BoxConstraints.debugAssertIsValid.<anonymous closure> (package:flutter/src/rendering/box.dart:540:19)
flutter: #2      BoxConstraints.debugAssertIsValid (package:flutter/src/rendering/box.dart:551:6)
flutter: #3      new ConstrainedBox (package:flutter/src/widgets/basic.dart:1884:27)
flutter: #4      _SuggestionsListState.build (package:flutter_typeahead/flutter_typeahead.dart:950:14)
flutter: #5      StatefulElement.build (package:flutter/src/widgets/framework.dart:3809:27)
flutter: #6      ComponentElement.performRebuild (package:flutter/src/widgets/framework.dart:3721:15)
flutter: #7      Element.rebuild (package:flutter/src/widgets/framework.dart:3547:5)
flutter: #8      ComponentElement._firstBuild (package:flutter/src/widgets/framework.dart:3701:5)
flutter: #9      StatefulElement._firstBuild (package:flutter/src/widgets/framework.dart:3848:11)
flutter: #10     ComponentElement.mount (package:flutter/src/widgets/framework.dart:3696:5)
flutter: #11     Element.inflateWidget (package:flutter/src/widgets/framework.dart:2950:14)
flutter: #12     Element.updateChild (package:flutter/src/widgets/framework.dart:2753:12)
flutter: #13     SingleChildRenderObjectElement.mount (package:flutter/src/widgets/framework.dart:4860:14)
flutter: #14     Element.inflateWidget (package:flutter/src/widgets/framework.dart:2950:14)
flutter: #15     Element.updateChild (package:flutter/src/widgets/framework.dart:2753:12)
flutter: #16     ComponentElement.performRebuild (package:flutter/src/widgets/framework.dart:3732:16)
flutter: #17     Element.rebuild (package:flutter/src/widgets/framework.dart:3547:5)
flutter: #18     ComponentElement._firstBuild (package:flutter/src/widgets/framework.dart:3701:5)
flutter: #19     ComponentElement.mount (package:flutter/src/widgets/framework.dart:3696:5)
flutter: #20     ParentDataElement.mount (package:flutter/src/widgets/framework.dart:4047:11)
flutter: #21     Element.inflateWidget (package:flutter/src/widgets/framework.dart:2950:14)
flutter: #22     Element.updateChild (package:flutter/src/widgets/framework.dart:2753:12)
flutter: #23     ComponentElement.performRebuild (package:flutter/src/widgets/framework.dart:3732:16)
flutter: #24     Element.rebuild (package:flutter/src/widgets/framework.dart:3547:5)
flutter: #25     ComponentElement._firstBuild (package:flutter/src/widgets/framework.dart:3701:5)
flutter: #26     StatefulElement._firstBuild (package:flutter/src/widgets/framework.dart:3848:11)
flutter: #27     ComponentElement.mount (package:flutter/src/widgets/framework.dart:3696:5)
flutter: #28     Element.inflateWidget (package:flutter/src/widgets/framework.dart:2950:14)
flutter: #29     Element.updateChild (package:flutter/src/widgets/framework.dart:2753:12)
flutter: #30     RenderObjectElement.updateChildren (package:flutter/src/widgets/framework.dart:4643:32)
@sjmcdowall
Copy link
Collaborator Author

Due to the prior error I had this code for the Typeahead:

...
        suggestionsBoxDecoration: SuggestionsBoxDecoration(
               constraints: BoxConstraints.expand(height: boxSize),
         ),
...

Where I had a calculation based on device height how much to reserve etc. for keyboard..

For this model (iPhone XR) -- I had --

Device Height is 896.0 -- calculated boxSize is 358.40000000000003

Not sure why that would blow up -- unless someone hard coded a value of 300? Which it appears to have done which -- as i think I pointed out -- isn't very good ..

My calculation had:

        final double boxSize =
            deviceHeight > 600 ? deviceHeight * 0.40 : deviceHeight * 0.25;

Which worked pretty well for me ..

@KaYBlitZ
Copy link
Contributor

KaYBlitZ commented Jan 8, 2019

Hi sjmcdowall. Thanks for finding the bug. The value 300 is hard-coded as the default value for the height. This value changes and is adjusted once the keyboard is shown and will do the calculation for the height to maximize it but still make it fit. Your code sets a minHeight less than 300 and crashes the code before it is able to auto-adjust.

I will add a quick check and add it to a PR.

You should be able to remove your suggestionsBoxDecoration to make it work properly.

@sjmcdowall
Copy link
Collaborator Author

@KaYBlitZ -- Yes I noticed that myself .. It is working pretty goods .. but .. if there is NO keyboard why limit it to 300? Is there not a way to use up "available" space -- or will it if the list is long enough?

@KaYBlitZ
Copy link
Contributor

KaYBlitZ commented Jan 8, 2019

As of right now, it will not use up the available space if there is no virtual keyboard, since the resize function is never called. Right now it is triggered by keyboard toggling. I can see if I can somehow calculate the exact height as the default instead of using a hard-coded value of 300.

@sjmcdowall
Copy link
Collaborator Author

Please see the following pics on an iPhone 6s .. Basically it could be a coding error on my part but when the suggestion list is shown .. I lose the ability to see / navigate to the "Previous / Next" buttons at the bottom of the screen .. also .. it's interesting that when I toggle the keyboard it DOES take up the full screen .. but once in full screen I can't figure out how to get rid of it (the suggestions) or see the buttons ..

Ideas?

screen shot 2019-01-08 at 11 16 44 am

screen shot 2019-01-08 at 11 16 58 am

screen shot 2019-01-08 at 11 17 35 am

screen shot 2019-01-08 at 11 17 50 am

@KaYBlitZ
Copy link
Contributor

KaYBlitZ commented Jan 8, 2019

Yes. Toggling the keyboard will call the resize function and adjust accordingly.

You can dismiss the suggestions list by pressing the enter key. This is the original functionality of the package and not something I have changed. AFAIK there is no other way of dismissing the list except selecting a suggestion.

@sjmcdowall
Copy link
Collaborator Author

@KaYBlitZ -- Ok -- that's pretty reasonable then. THanks..

@KaYBlitZ
Copy link
Contributor

KaYBlitZ commented Jan 8, 2019

@sjmcdowall I think I was able to fix all the issues. I put them all in a PR. You can test the potential changes by editing in your pubspec.yaml

flutter_typeahead:
    git: git://github.com/KaYBlitZ/flutter_typeahead.git

I think the plugin works the way it should now. The initial height is calculated during creation now, so the static height of 300 should never be used.

@sjmcdowall
Copy link
Collaborator Author

Great -- I'll give it a shot ASAP and report back ..

@sjmcdowall
Copy link
Collaborator Author

sjmcdowall commented Jan 8, 2019

screen shot 2019-01-08 at 12 50 29 pm

OK -- This is working really well except for a minor nit -- see attached screen shot and you'll see on an iPhone X type device the box extends into the "Unsafe" area .. I've solved this before with logic like:
        Container(
          padding: EdgeInsets.only(
              bottom: max(0.0, MediaQuery.of(context).padding.bottom - 15.0)),
          child: _buildPageViewIndicator(),
        )

Here is an except from the Flutter issue on this -- there is now also a SafeArea widget to protect against going against either the top or bottom (or I guess side?) areas on devices and it's "smart" to know what to do (in theory anyway) .. :)

@deborah-ufw manual iPhone X detection should no longer be necessary. The framework now includes support for iOS safe area insets. See the SafeArea class for details. If, rather than automatically applying padding, you need the pixel values of the safe area insets, they've available via MediaQuery.of(context).padding. .bottom for the iPhone X home indicator safe area height, .left/.right for the notch in landscape mode (and symmetric inset on the other side), .top for the status bar/notch in portrait mode.

As a bonus, these will work with all future iPhone X-like iPhone models, as well as any upcoming 'creative' displays on Android phones as well -- the only one I'm currently aware of is the Essential Phone, which has a camera notch, but I'm sure there'll be more.

@KaYBlitZ
Copy link
Contributor

KaYBlitZ commented Jan 8, 2019

Got it. Thanks for the info. Seems like an easy fix. Just have to use MediaQuery. I'll add and test it later when I have time.

@deborah-ufw
Copy link

Awesome - thank you!!

@KaYBlitZ
Copy link
Contributor

KaYBlitZ commented Jan 9, 2019

@sjmcdowall Finally got everything working. Orientation changes and SafeArea considerations should all be good. If you are curious, I put many comments in the code to explain how it works. Please test it when you have time.

@sjmcdowall
Copy link
Collaborator Author

@KaYBlitZ -- I ran it throw some good tests -- iPhone X/6S and Android 6/6P and it looks and works great! Nice work! Now -- how to get this PR into main :)

@AbdulRahmanAlHamali -- perhaps you should add @KaYBlitZ to the offical commit team ?? A good OS project needs to have more than just one authorized merger/commiter / pusher IMHO

@AbdulRahmanAlHamali
Copy link
Owner

You're right @sjmcdowall, @KaYBlitZ is doing a lot of good work, and the package needs a more active contributor since I am unable to give the required time.

@KaYBlitZ would you like to become a team member?

@sjmcdowall
Copy link
Collaborator Author

@AbdulRahmanAlHamali -- @KaYBlitZ better accept or else I'll do something evil! :) If you want another backup -- I wouldn't mind being a member of the team .. I'm a good code reviewer if not the ultimate flutter coder .. :)

@KaYBlitZ
Copy link
Contributor

Sure, I don't mind. Pretty new to this. Is there a process for the way commits and PRs are handled?

@sjmcdowall
Copy link
Collaborator Author

sjmcdowall commented Jan 16, 2019

Did this get figured out @KaYBlitZ and @AbdulRahmanAlHamali ?? I don't think the latest changes have been pushed to master / dart pub?

@KaYBlitZ
Copy link
Contributor

No, I haven't heard anything since. I am starting to be more busy myself, so it might be better this way. The plugin still works fine except for the recent type error discussion and the option of the listing being on top.

@AbdulRahmanAlHamali
Copy link
Owner

Hello,
I apologize but I was travelling the past few days. I will add you both as committers, that way whoever has the time can follow up with the open issues and pull requests.

Regarding the review of pull requests. You need to do the following:

  • only merge into develop, not into master. If a pull request is sent for master, modify it for develop.
  • review the code and test it well: does it work? Does it break anything? Is it clean and well-structured? Etc. If you find something unsatisfactory you could ask the committer to fix it.
  • make sure that the pull request only changes what it is supposed to change. Do not accept requests that have changes all over the place.

After you merge into develop. You should update the version:

  • if it is only a bug fix, increase the last digit.
  • if it provides a new feature, increase the middle digit.
  • if it has breaking changes, you should increase the first digit.

Then, mention all the changes that you did in the CHANGELOG, and you can merge develop then into master.

Finally, you should inform me to push the new version to the pub website, and that's it!

@sjmcdowall
Copy link
Collaborator Author

sjmcdowall commented Jan 18, 2019 via email

@AbdulRahmanAlHamali
Copy link
Owner

Actually it seems you guys can upload too. I'm not sure how this works exactly, but we can try to let someone else do the publishing when we're releasing the next version!

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

No branches or pull requests

4 participants