flutter / flutter

Flutter makes it easy and fast to build beautiful apps for mobile and beyond
https://flutter.dev
BSD 3-Clause "New" or "Revised" License
165.45k stars 27.31k forks source link

Add an option to set a Dismissble as 'active' #24713

Open diablodale opened 5 years ago

diablodale commented 5 years ago

Attempt to disable the Dismissible() behavior by setting .direction = null causes assert at https://github.com/flutter/flutter/blob/6d134e0c866ce90a4dc7a02af6a8f1079c2a4fdc/packages/flutter/lib/src/widgets/dismissible.dart#L337

This is a feature enhancement to simplify customer code, not a code fault.

Setup

Flutter version 0.11.9 Framework revision d48e6e433c (5 days ago), 2018-11-20 22:05:23 -0500 Engine revision 5c8147450d Dart version 2.1.0 (build 2.1.0-dev.9.4 f9ebf21297) Platform android-27, build-tools 27.0.3

Repo Scenario

It would be desired to use the Dismissible.direction field to set the means to be dismissed. With flutter v0.11.9, we can set that field to any choice of DismissDirection but unfortunately not to null. Naturally, null as a .direction would indicate there is no direction in which it can be dismissed...making the Dismissible a noop.

Support for null as a value to Dismissible.direction will simply customer code. Rather than using a condition expression and duplicating the child widget code, it can more easily be done by just setting the direction to null.

Today's unhappy duplicate code 😐

Listview.builder(...
  itemBuilder:
    return isDisabled ?
      CardWidgetAndCode(
        ...
      )
    : Dismissible(
      child: CardWidgetAndCode( //  <-- duplicate code and/or function call
        ...
      )
    )
)

Future happy simplified code πŸ™‚

Listview.builder(...
  itemBuilder:
    Dismissible(
      direction: isDisabled ? null : DismissDirection.horizontal,
      child: CardWidgetAndCode(
        ...
      )
    )
)

Log of assert

package:flutter/src/widgets/dismissible.dart': Failed assertion: line 337 pos 12: 'widget.direction != null': is not true

Doctor

[√] Flutter (Channel beta, v0.11.9, on Microsoft Windows [Version 10.0.17763.165], locale en-US)
    β€’ Flutter version 0.11.9 at C:\Users\dale\.flutter
    β€’ Framework revision d48e6e433c (5 days ago), 2018-11-20 22:05:23 -0500
    β€’ Engine revision 5c8147450d
    β€’ Dart version 2.1.0 (build 2.1.0-dev.9.4 f9ebf21297)

[√] Android toolchain - develop for Android devices (Android SDK 27.0.3)
    β€’ Android SDK at C:\Users\dale\.android\Sdk
    β€’ Android NDK location not configured (optional; useful for native profiling support)
    β€’ Platform android-27, build-tools 27.0.3
    β€’ ANDROID_HOME = C:\Users\dale\.android\Sdk
    β€’ Java binary at: C:\Program Files\Android\android-studio-preview\jre\bin\java
    β€’ Java version OpenJDK Runtime Environment (build 1.8.0_152-release-1136-b06)
    β€’ All Android licenses accepted.

[√] Android Studio (version 3.2)
    β€’ Android Studio at C:\Program Files\Android\android-studio-preview
    β€’ Flutter plugin version 30.0.1
    β€’ Dart plugin version 181.5656
    β€’ Java version OpenJDK Runtime Environment (build 1.8.0_152-release-1136-b06)

[√] VS Code, 64-bit edition (version 1.29.1)
    β€’ VS Code at C:\Program Files\Microsoft VS Code
    β€’ Flutter extension version 2.20.0

[√] Connected device (1 available)
    β€’ Android SDK built for x86 β€’ emulator-5554 β€’ android-x86 β€’ Android 8.0.0 (API 26) (emulator)

β€’ No issues found!
Sub6Resources commented 5 years ago

Created a pull request for this issue: #36613

Piinks commented 4 years ago

@goderbauer and I chatted offline about making this change and we have closed the associated PR. Using a Dismissible widget should be dismissible, and making the direction nullable creates a weird api without a clear intention. If you would like to simplify code to 'enable/disable' the Dismissible, you can easily create a wrapper widget that handles that for you. :) I'm going to close this issue then, feel free to discuss further if you think the assert is incorrect.

diablodale commented 4 years ago

I disagree with your assessment. However without the above conversation being public, there is no way that I can participate in it.

Sub6Resources commented 4 years ago

I also disagree and would also have appreciated it if the discussion happened on GitHub.

Piinks commented 4 years ago

Glad to hear! The discussion actually took place on the PR: #48921 The extent of our offline chat was to just close the issue given the comments on the PR. There are a number of concerns around removing the assert on the direction, as it introduces the opportunity for exceptions to fire elsewhere that rely on that assertion. Feel free to discuss further! :)

Piinks commented 4 years ago

Based on the PR feedback, I'm going to re-open this and re-name it for the newly proposed api change. Feel free to continue discussing here and thanks for the input! :)

bounty1342 commented 4 years ago

Hi,

Why not just add a DismissDirection.none ?

Regards,

HelgeSverre commented 1 week ago

Regarding the code duplication issue mentioned earlier, here's a simple technique that can be used as an alternative:

class ExampleWidget extends StatelessWidget {
  final DismissDirectionCallback? onDismissed;
  final String text;

  const ExampleWidget({
    required this.text,
    this.onDismissed,
    super.key,
  });

  @override
  Widget build(BuildContext context) {
    // Define the core widget
    var item = Container(
      padding: const EdgeInsets.all(10),
      color: Colors.blue,
      child: Text(text, style: const TextStyle(color: Colors.black)),
    );

    // Conditionally wrap in Dismissible
    if (onDismissed != null) {
      return Dismissible(
        key: ObjectKey(text),
        onDismissed: onDismissed,
        child: item,
      );
    }

    return item;
  }
}

This approach demonstrates how to avoid duplicating widget code when conditionally applying a Dismissible wrapper. By assigning the core widget to a variable first, we can easily return either the wrapped or unwrapped version without repeating the widget's definition.

While this doesn't solve the original issue, I think it is a somewhat neat and clean way to achieve effectively the same thing.

If anyone knows of any reasons why this approach should not be used (e.g., performance issues, potential bugs this might cause), please feel free to correct me.

diablodale commented 1 week ago

@HelgeSverre thanks for the interesting example. I don't have a current Flutter project to experiment with it. The issue seems to be one that appears more than once, and your example may be workaround for the ongoing core gap in feature/functionality.