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

[flutter_splash_tizen] Initial implementation #266

Closed
wants to merge 39 commits into from

Conversation

JRazek
Copy link

@JRazek JRazek commented Nov 5, 2021

Issue #151
Adds support for native splash as a flutter package.

@github-actions github-actions bot added the needs-publishing The package should be published after merge label Nov 5, 2021
@pkosko pkosko requested a review from a team November 5, 2021 10:36
@JRazek JRazek changed the title flutter splash for tizen flutter splash for tizen issue #151 Nov 5, 2021
@JRazek JRazek changed the title flutter splash for tizen issue #151 flutter splash for tizen Nov 5, 2021
uses-material-design: true

flutter_splash_tizen:
image: assets/test.png
Copy link
Contributor

Choose a reason for hiding this comment

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

no new line at the end of file

Copy link
Contributor

Choose a reason for hiding this comment

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

one more comment - I assume that the example application is after adding a splash file with flutter-tizen pub run flutter_splash_tizen:create because related line is added to tizen-manifest.xml. But file assets/test.png does not exist, so removing splash and re-adding it again will probably fail - please check it.

I am not also sure if packages/flutter_splash_tizen/example/tizen/shared/res/test.png file will work as a splash screen, as the tizen-manifest file uses: assets/test.png.

// Platform messages may fail, so we use a try/catch PlatformException.
// We also handle the message potentially returning null.
try {
platformVersion =
Copy link
Contributor

Choose a reason for hiding this comment

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

is the platformVersion really a part of splash plugin? or is it just leftover after some base for development?

dev_dependencies:
flutter_test:
sdk: flutter
flutter_lints: ^1.0.0
Copy link
Contributor

Choose a reason for hiding this comment

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

new line missing at the end of file

channel.setMockMethodCallHandler(null);
});

test('getPlatformVersion', () async {
Copy link
Contributor

Choose a reason for hiding this comment

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

are these tests related to splash screen?

import 'package:flutter/services.dart';

class FlutterNativeSplashTizen {
static const MethodChannel _channel =
Copy link
Contributor

Choose a reason for hiding this comment

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

is it related to splash screen?

import 'package:xml/xml.dart';

void main() {
var doc = loadYamlFileSync("pubspec.yaml")?['flutter_splash_tizen'];
Copy link
Contributor

Choose a reason for hiding this comment

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

if I understand correctly, current flow supports only adding a file once, then file added to yaml will be copied to tizen-manifest. What about changing a file mentioned in yaml during the development? Shouldn't we consider a scenario that a file is added to manifest file everytime when application is built which means that the file used in tizen manifest will be kept in sync with file mentioned in pubspec.yaml file?

Copy link
Contributor

Choose a reason for hiding this comment

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

@swift-kim could you please share your opinion regarding this comment? Should we set splash screen 'on demand' only, or rather do it automatically on every build?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's better to sync automatically every build if possible. By the way, is there a way to run this package every build?
Should we add this feature to flutter-tizen tool?

Copy link
Contributor

Choose a reason for hiding this comment

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

doc can be null. please check it.

Copy link
Member

Choose a reason for hiding this comment

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

@pkosko You can just follow flutter_native_splash's way. So manually running the pub run command every time the attributes are changed should just be enough.

uses-material-design: true

flutter_splash_tizen:
image: assets/test.png
Copy link
Contributor

Choose a reason for hiding this comment

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

one more comment - I assume that the example application is after adding a splash file with flutter-tizen pub run flutter_splash_tizen:create because related line is added to tizen-manifest.xml. But file assets/test.png does not exist, so removing splash and re-adding it again will probably fail - please check it.

I am not also sure if packages/flutter_splash_tizen/example/tizen/shared/res/test.png file will work as a splash screen, as the tizen-manifest file uses: assets/test.png.

@swift-kim
Copy link
Member

By the way this package is not a plugin. I need to make a decision on which repository this package should be placed in.

@swift-kim
Copy link
Member

I discussed with @WonyoungChoi and we decided to keep this package in this repo for now. The decision may be changed in the future.

packages/flutter_splash_tizen/bin/file_utils.dart Outdated Show resolved Hide resolved
packages/flutter_splash_tizen/bin/file_utils.dart Outdated Show resolved Hide resolved
packages/flutter_splash_tizen/bin/file_utils.dart Outdated Show resolved Hide resolved
import 'package:xml/xml.dart';

void main() {
var doc = loadYamlFileSync("pubspec.yaml")?['flutter_splash_tizen'];
Copy link
Contributor

Choose a reason for hiding this comment

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

unnecessary loading yaml file.

import 'package:xml/xml.dart';

void main() {
var doc = loadYamlFileSync("pubspec.yaml")?['flutter_splash_tizen'];
Copy link
Contributor

Choose a reason for hiding this comment

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

doc can be null. please check it.

Copy link
Member

@swift-kim swift-kim left a comment

Choose a reason for hiding this comment

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

Issue:
A blank (black) screen is displayed before the first app frame is drawn and after the splash screen is displayed.

Additional work:

packages/flutter_splash_tizen/.metadata Outdated Show resolved Hide resolved
packages/flutter_splash_tizen/CHANGELOG.md Outdated Show resolved Hide resolved
packages/flutter_splash_tizen/pubspec.yaml Outdated Show resolved Hide resolved
packages/flutter_splash_tizen/LICENSE Outdated Show resolved Hide resolved
packages/flutter_splash_tizen/LICENSE Outdated Show resolved Hide resolved
packages/flutter_splash_tizen/pubspec.yaml Outdated Show resolved Hide resolved
packages/flutter_splash_tizen/pubspec.yaml Outdated Show resolved Hide resolved
packages/flutter_splash_tizen/pubspec.yaml Outdated Show resolved Hide resolved
@@ -0,0 +1,13 @@
<?xml version="1.0" encoding="utf-8"?>
<manifest package="com.example.flutter_splash_tizen_example" version="1.0.0" api-version="4.0" xmlns="http://tizen.org/ns/packages">
<profile name="wearable"/>
Copy link
Member

Choose a reason for hiding this comment

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

Does this package support TV as well?

Copy link
Member

Choose a reason for hiding this comment

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

Could you confirm whether splash screen is supported on TV devices or not? If not, you should mention the fact in the package's README.

Copy link
Author

Choose a reason for hiding this comment

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

@swift-kim As far as I see - it does not run on TV. It runs on mobile and on Wearable fine.

Copy link
Author

Choose a reason for hiding this comment

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

Will this work for that?

Package has been tested on Mobile 6.0, Wearable 6.0 and IoT Headed 6.5 platform.

@HakkyuKim
Copy link
Contributor

The CI will pass when you rebase to master branch.

Comment on lines 13 to 18
String tizenManifestPath = "tizen/tizen-manifest.xml";

XmlDocument? tizenManifest = loadXMLFileSync(tizenManifestPath);
if (tizenManifest == null) {
throw const FormatException("could not read tizen-manifext.xml!");
}
Copy link
Contributor

@WonyoungChoi WonyoungChoi Nov 17, 2021

Choose a reason for hiding this comment

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

Unnecessary exporting the method loadXMLFileSync.

Suggested change
String tizenManifestPath = "tizen/tizen-manifest.xml";
XmlDocument? tizenManifest = loadXMLFileSync(tizenManifestPath);
if (tizenManifest == null) {
throw const FormatException("could not read tizen-manifext.xml!");
}
File tizenManifestFile = File("tizen/tizen-manifest.xml");
if (!file.existsSync()) {
throw const FormatException("could not read tizen-manifext.xml!");
}
XmlDocument? tizenManifest = XmlDocument.parse(tizenManifestFile.readAsStringSync());

@swift-kim
Copy link
Member

Did you read all my comments in the above?

@github-actions github-actions bot added the tools label Nov 17, 2021
@JRazek JRazek closed this Nov 17, 2021
@JRazek JRazek reopened this Nov 17, 2021
@swift-kim
Copy link
Member

Hey. There are more comments.

@swift-kim
Copy link
Member

swift-kim commented Nov 26, 2021

Are you serious? Why are you publishing a package which isn't even merged?

Well, publishing cannot be undone. We can still review this PR until until it becomes mergeable. Any new changes will be published in the next release.

@swift-kim
Copy link
Member

Issue:
A blank (black) screen is displayed before the first app frame is drawn and after the splash screen is displayed.

This issue needs a fix before this package can be merged.

@swift-kim swift-kim changed the title flutter splash for tizen [flutter_splash_tizen] Initial implementation Dec 3, 2021
@bbrto21
Copy link
Contributor

bbrto21 commented Dec 9, 2021

Issue:
A blank (black) screen is displayed before the first app frame is drawn and after the splash screen is displayed.

This issue needs a fix before this package can be merged.

I opened new PR to fix this issue.
@JRazek Could you check it using this PR?

@bbrto21
Copy link
Contributor

bbrto21 commented Dec 10, 2021

I opened new PR to fix this issue. @JRazek Could you check it using this PR?

There's still a blank screen between the splash image and the first frame :(

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.

6 participants