bryanoltman / stager

Apache License 2.0
21 stars 5 forks source link

Stager generator detects only one scene, when there are two. #14

Closed polina-c closed 1 year ago

polina-c commented 2 years ago

Scenes:

import 'package:flutter/material.dart';
import 'package:stager/stager.dart';

/// To run:
/// flutter run -t test/scenes/hello.stager_app.dart -d macos  --dart-define='Scene=HelloScene'
class HelloScene extends Scene {
  @override
  Widget build() {
    return MaterialApp(
      home: Card(
        child: Text('Hello, I am $title.'),
      ),
    );
  }

  @override
  Future<void> setUp() async {}

  @override
  String get title => '$runtimeType';
}

/// To run:
/// flutter run -t test/scenes/hello.stager_app.dart -d macos  --dart-define='Scene=HelloSceneDerived'
class HelloSceneDerived extends HelloScene {}

generated file:

// GENERATED CODE - DO NOT MODIFY BY HAND

// **************************************************************************
// StagerAppGenerator
// **************************************************************************

import 'hello.dart';

import 'package:stager/stager.dart';

void main() {
  final scenes = [
    HelloScene(),
  ];

  if (const String.fromEnvironment('Scene').isNotEmpty) {
    const sceneName = String.fromEnvironment('Scene');
    final scene = scenes.firstWhere((scene) => scene.title == sceneName);
    runStagerApp(scenes: [scene]);
  } else {
    runStagerApp(scenes: scenes);
  }
}
bryanoltman commented 2 years ago

This is by design (only Scenes that implement title are placed in the generated file), but seeing as this behavior is fairly arbitrary and the thinking behind it was not informed by actual usage, I see no reason why this behavior couldn't be changed. Even if not, it could certainly stand to be documented :)

A few questions:

  1. Should we do codegen in the first place? How much value do you feel you get out of the generated code? I've found myself not really using it much in my own work because the generated code mostly amounts to runApp(StagerApp(scenes:[...])). Maybe we could just get rid of codegen altogether?
  2. If we keep codegen, the "scenes only show up if they have a title" behavior is not especially obvious, and I'd be curious to hear any thoughts or suggestions you have about a more intuitive way to communicate whether a scene should be displayed. Maybe "all non-abstract Scene classes are displayed"? What do you think?
polina-c commented 2 years ago

It seems without codegen I need to modify code to do this, it will add friction, because it is much faster and takes less mental effort to copy-paste the command to console, than to open IDE, find the right file and make right edits. So, yes, codegen is valuable from my perspective.

polina-c commented 2 years ago

However, it is ok to declare that one file should contain only one scene, if it is easier to maintain.

bryanoltman commented 2 years ago

one file should contain only one scene

That's not the issue here. If your code above implemented title in HelloSceneDerived, HelloSceneDerived would be placed in the Scene list.

polina-c commented 2 years ago

one file should contain only one scene

That's not the issue here. If your code above implemented title in HelloSceneDerived, HelloSceneDerived would be placed in the Scene list.

HelloSceneDerived has title equal to HelloSceneDerived, as the title is based on runtime type. What would be different if title was implemented explicitly?

bryanoltman commented 2 years ago

Yep, that's right – title has to be implemented explicitly. As I said earlier though, I don't feel like this is the best or most obvious way for a developer to signify which scenes they want to show or not show, so looking for ways to improve. Ideas:

  1. All non-abstract Scene subclasses are displayed.
  2. Add a showInApp property to Scene.
  3. Some combination of the above (showInApp defaults to false, all non-abstract Scene subclasses are shown in the Stager app unless showInApp is true).
bryanoltman commented 1 year ago

Closing this issue. Let's open a separate improvement request ticket since this is working as intended.