-
-
Notifications
You must be signed in to change notification settings - Fork 105
feat: run on desktop #678
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
base: master
Are you sure you want to change the base?
feat: run on desktop #678
Conversation
|
Checked the failures:
|
faf7008 to
d5e03c9
Compare
|
Hi @mateusz-bak, could you please help reviewing this and couple more PRs? Or maybe there is someone else with write permissions who could help? |
a63eec2 to
6645da5
Compare
6645da5 to
c68f954
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change already made, please rebase
pubspec.yaml
Outdated
| file_picker: ^8.3.1 | ||
| smooth_page_indicator: ^1.1.0 | ||
| intl: ^0.19.0 | ||
| intl: ^0.20.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change already made, please rebase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change already made, please rebase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! Thanks for the work, looks like a good improvement.
However I don't see linux or windows directories that I suppose were created after running below command:
flutter create --platforms=linux,windows .
Also please rebase to fix conflicts on already done changes.
c68f954 to
a233e5f
Compare
|
Hi! I would like give some help to merge this PR. Could you give me an update on the current status of the PR and any blockers you’re facing? Thanks in advance :) |
a233e5f to
006403c
Compare
|
Hi, I've rebased the changes on top of the current HEAD and added the linux platform directory. Regarding windows platform directory and build, I didn't add it because I haven't tested it there and won't be able to. One more thing that is missing yet is an automatic build configuration for linux. We won't be able to distribute the linux version without that. |
|
Hi! Has it evolved since last month?? (I can't wait to get OpenReads on my desktop bc i don't have a smartphone anymore) |
73e8188 to
a571bef
Compare
It includes sql configuration for windows, but it's testted only on Linux. Got an example from here: https://github.com/alextekartik/flutter_app_example/blob/96cba15d72e82ad739323f489b261014e3854484/demo_sqflite/lib/main.dart#L15-L21
def18f8 to
837978c
Compare
|
BTW folks. While we're waiting for review you can try it out using the build artifacts: https://github.com/mateusz-bak/openreads/actions/runs/19399497883 To use it:
I did fix everything platform-specific I could fine, but I might've missed something. So let me know if it's crashing or silently ignoring you. CC: @armand-leguillou, @amoinier, @ironhak, @GLLM |
|
Great work @RomanKovtyukh I'm starting the review, I'll try to finish it by tomorrow. |
| _startCreatingCloudBackup(context) async { | ||
| if (!(Platform.isAndroid || Platform.isIOS)) { | ||
| BackupGeneral.showInfoSnackbar( | ||
| 'Cloud backup is not implemented for ${Platform.operatingSystem} yet.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shows "Cloud backup is not implemented for linux yet."
I think it should be "Linux"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean "linux" not being with capital "L"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's coming from the Platform package, so we'd have to capitalize it ourselves as far as I understand or submit a patch to the Platform package. Don't know which is better TBH.
Hardcoding the whole string with "Linux" would mean that if we want to run it on Windows we'll have to add a separate if statement for that which I'd like to avoid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I replied by mail, and my messages have been appended to the main thread instead of this one. I wasn't aware of that behaviour, sorry. I'm posting them here now, and removing from the main one.
Do you mean "linux" not being with capital "L"?
Yes, exactly. Sorry that I wasn't precise!
It's coming from the Platform package, so we'd have to capitalize it ourselves as far as I understand or submit a patch to the Platform package. Don't know which is better TBH.
Hardcoding the whole string with "Linux" would mean that if we want to run it on Windows we'll have to add a separate if statement for that which I'd like to avoid.
I imagine this pull request will take some days before it can be merged. So how about submitting a patch, and if it's not accepted by the time we're ready to merge, we'll capitalise ourselves? I'd definitely not hardcode it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move string "Cloud backup is not implemented for" to translations file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I'm not sure what is the status of the PR in the package but I'm for leaving how it is in the package and waiting for fix.
Thanks a lot for your work! 👏 Here's some feedback from a short session of looking around. Although there are some issues, it's usable to a high degree, and I'm very happy to have this awesome app available on Linux 🤩 This is just great! Functional issuesThese are issues that I think would be good to fix before releasing. Note that I'm stuck with Android version 2.9.0 so I can't check exactly if they appear only on the desktop version.
Possible UI improvementsThese issues are related to migrating the UI of a mobile app to the desktop. In my opinion, they can wait until after the first release.
|
|
Thanks for the review, @mbeko. Really good findings you have there.
I'm kind of able to reproduce this or similar issue sometimes: openreads is saying "the file is not a valid opereads backup", but then it's working perfectly fine on subsequent runs. Debugging showed that the file is really the right backup, so we must've returned
Sure, need to fix it before releasing.
Yeah, need to fix this. I've seen it, but didn't pay attention since I didn't wait long enough to notice the CPU consumption.
Agree
Good point. I need to check since I've seen files in
Yeah, had the same trouble. On the release builds it's working well -- just no "Debug" indicator. Still need to check the rest of the issues you mentioned. Thanks again for the review. |
ok, this is definitely a race condition of _checkInfoFileVersion with Reproduced this way:
Added commit where I'm adding a concept of an "internal error" asking users to report what happened. It shouldn't be this rushed and shouldn't be part of this PR in my opinion, so I'd like to hear @mateusz-bak opinion on this. But anyway I'd keep the |
020235d to
dd076b1
Compare
|
Made the extraction fully synchronous everywhere. In the Made it so because even after extracting |
dd076b1 to
723a2f6
Compare
fixed: I was returning from the function without setting state back to |
Added the same "not implemented" note to all. |
Make unzipping the archive synchronous to avoid reading files from backup before they got extracted. In the backup v5 the `info.txt` file is getting extracted first to avoid extracting the whole archive before it's confirmed to be a valid backup.
723a2f6 to
f1d3a61
Compare
Just disabled it for now: it'll not show any pop-up, but won't crash at least.
Haven't checked yet, only made it show "unimplemented" warning.
Fixed this: |
f1d3a61 to
90c524a
Compare
|
Great, thanks a lot! I'll try to test this on the weekend |
|
I can confirm that all the changes you mentioned work as described 💪 🙇 |
|
Sorry, missed this in the last message:
This is working same in 2.11 on my Android device. Do you have it working differently on non-Linux? If yes, could you please share the version and the platform? The rest is either blocked or fixed, so I'd ask @mateusz-bak to make a call if it's ready to be merged to main branch. I'd personally prefer merging it only to avoid rebases/merges with the newer main branch versions. |
|
Ah, I see. I'm unfortunately stuck with 2.9.0 on Android so I can't compare, but then it's likely that it's not an issue specific to the desktop version. Thanks! |
mateusz-bak
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RomanKovtyukh awesome work! I'm very sorry about the delay (I'm only starting to acclimate to new reality of life with a newborn).
I left some comments about the code - couple of simple fixes that I would like to see. I'll go through comments on the PR and will gather my thought about the Linux specific stuff and UI/UX.
But I think after the requested changes we can merge this PR and have a list of things to improve in the Linux version.
Again great work, I really appreciate it!
| await CSVImportOpenreads.importCSV(context); | ||
| } else { | ||
| BackupGeneral.showInfoSnackbar( | ||
| 'Openreads CSV import is not implemented for ${Platform.operatingSystem} yet.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move string to translations file
| await CSVImportGoodreads.importCSV(context); | ||
| } else { | ||
| BackupGeneral.showInfoSnackbar( | ||
| 'Goodreads CSV import is not implemented for ${Platform.operatingSystem} yet.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move string to translations file
| await CSVImportBookwyrm.importCSV(context); | ||
| } else { | ||
| BackupGeneral.showInfoSnackbar( | ||
| 'BookWyrm CSV import is not implemented for ${Platform.operatingSystem} yet.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move string to translations file
| Constants.blurHashY, | ||
| ); | ||
| } else { | ||
| throw Exception('Unsupported platform for generating BlurHash'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move string to translations file
|
|
||
| Future<Database> createDatabase() async { | ||
| Directory docDirectory = await getApplicationDocumentsDirectory(); | ||
| Directory docDirectory = Directory('${dataHome.path}/openreads'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As package xdg_directories supports only Linux maybe we should set docDirectory as:
Directory docDirectory;
if (Platform.isLinux) {
docDirectory = Directory('${dataHome.path}/openreads');
} else {
docDirectory = await getApplicationDocumentsDirectory();
}
| _startCreatingCloudBackup(context) async { | ||
| if (!(Platform.isAndroid || Platform.isIOS)) { | ||
| BackupGeneral.showInfoSnackbar( | ||
| 'Cloud backup is not implemented for ${Platform.operatingSystem} yet.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move string "Cloud backup is not implemented for" to translations file.
| _startCreatingCloudBackup(context) async { | ||
| if (!(Platform.isAndroid || Platform.isIOS)) { | ||
| BackupGeneral.showInfoSnackbar( | ||
| 'Cloud backup is not implemented for ${Platform.operatingSystem} yet.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I'm not sure what is the status of the PR in the package but I'm for leaving how it is in the package and waiting for fix.
| } | ||
|
|
||
| Future<void> _scanBarcode() async { | ||
| if (!Platform.isAndroid && !Platform.isIOS) return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of doing that please modify lib/ui/home_screen/widgets/add_book_sheet.dart
and make below code hidden on platforms other than Android and iOS until this is implemented:
ListTile(
title: Text(LocaleKeys.add_scan.tr()),
leading: FaIcon(
FontAwesomeIcons.barcode,
color: Theme.of(context).colorScheme.primary,
),
onTap: widget.scanBarcode,
),
| } | ||
|
|
||
| void _startScanner() async { | ||
| if (!Platform.isAndroid && !Platform.isIOS) return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of doing that please modify build method
and make below code of the IconButton hidden on platforms other than Android and iOS until this is implemented:
actions: [
IconButton(
onPressed: _startScanner,
icon: const FaIcon(FontAwesomeIcons.camera, size: 18),
),
],
| borderRadius: BorderRadius.circular(cornerRadius), | ||
| ), | ||
| onTap: () async { | ||
| if (!Platform.isAndroid && !Platform.isIOS) return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of doing that please hide the InkWell widget on platforms other than Android and iOS until this is implemented.
@mbeko this is exactly how it is implemented right now. From Book Screen you can click on tags to show list of books that have the tag assigned. From Home Screen the tags are not clickable. If you think it should be added, please raise as a feature request in the Discussions 💙 |
There are some issues with the backups but I agree they should not be a part of the PR. |
|
Stuff to be added, to make the app better suited for Linux and general desktop experience:
If you have more points, please write! |
It includes sqlite configuration for windows, but only tested on Linux. Got an example from here so decided not to remove the
Platform.isWindowsfrom there.Before this change openreads was showing the following errors at the startup if you try running it on desktop:
#584