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

Refactor logging system to use Hive for log storage and metadata managment. #1016

Draft
wants to merge 8 commits into
base: redesign
Choose a base branch
from

Conversation

aman-boop
Copy link

the Setup_loggin.dart now uses the metadata_helper.dart to fetch info

…gement; add metadata helper for device and app information
@aman-boop aman-boop changed the base branch from redesign to main January 12, 2025 15:11
@aman-boop aman-boop changed the base branch from main to redesign January 12, 2025 15:12
@aman-boop
Copy link
Author

@Chaphasilor I don't know, I changed the target branch

Copy link
Collaborator

@Chaphasilor Chaphasilor left a comment

Choose a reason for hiding this comment

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

looks mostly good, but haven't tried it out yet. commented some minor changes, but this is definitely headed in the right direction! 😁

lib/services/finamp_logs_helper.dart Show resolved Hide resolved
lib/services/finamp_logs_helper.dart Outdated Show resolved Hide resolved
lib/services/finamp_logs_helper.dart Outdated Show resolved Hide resolved
lib/services/finamp_logs_helper.dart Outdated Show resolved Hide resolved
lib/services/finamp_logs_helper.dart Show resolved Hide resolved
lib/setup_logging.dart Show resolved Hide resolved
lib/setup_logging.dart Show resolved Hide resolved
@aman-boop
Copy link
Author

I can't test it out myself

Copy link
Collaborator

@Chaphasilor Chaphasilor left a comment

Choose a reason for hiding this comment

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

Nice progress! Exporting the zip file works, but there are some additional changes needed :)


void main() async {
// If the app has failed, this is set to true. If true, we don't attempt to run the main app since the error app has started.
bool hasFailed = false;
try {
await setupLocator();
Copy link
Collaborator

Choose a reason for hiding this comment

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

please either namespace this (import 'services/metadata_helper.dart' as metadataHelper; and then await metadataHelper.setupLocator()), or move the setup function to main.dart (like all the other setup functions).

Comment on lines +152 to +155
getIt.registerLazySingleton(() => DeviceInfo());
getIt.registerLazySingleton(() => AppInfo());
getIt.registerLazySingleton(() => ServerInfo());
getIt.registerLazySingleton(() => MetaData());
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think it's necessary to register four new global singletons. The MetaData class should be enough, and you could add any methods that need to be accessed into it. Or provide getters for the other classes through MetaData.

Also, maybe rename it to MetaDataHelper?

}
startupLogger.info("Running on $deviceInfoString.");
startupLogger.info(
"This is ${packageInfo.version} version ${packageInfo.version}+${packageInfo.originallyInstalledVersion} (Signature '${packageInfo.installationDate}'), installed via ${packageInfo.updateHistory}.");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please don't change the information that is logged. The missing information (like appName, buildNumber, etc. should be added to the AppInfo class)

WindowsDeviceInfo windowsInfo = await deviceInfo.windowsInfo;
osVersion = windowsInfo.displayVersion;
deviceModel = windowsInfo.deviceId;
deviceType = "PC"; // You can add more logic to determine the device type
Copy link
Collaborator

Choose a reason for hiding this comment

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

how about you just add platform instead of device type, and set them accordingly (Android, Windows, Linux, etc.)?

Comment on lines +142 to +146
return {
"appInfo": appInfo.toJson(),
"serverInfo": serverInfo.toJson(),
"deviceInfo": deviceInfo.toJson(),
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't generate JSON. The resulting string has no quotes at all. Please use the @JsonSerializable annotation and toJSON method like it's done in finamp_models.dart. You'll have to run code generation after adding the annotation. You can do that by running dart run build_runner build --delete-conflicting-outputs.

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.

2 participants