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

Implement HiveBuilder, obey lints #25

Closed
wants to merge 6 commits into from
Closed

Implement HiveBuilder, obey lints #25

wants to merge 6 commits into from

Conversation

SarcasticNoodle
Copy link

No description provided.

Copy link
Member

@simc simc left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request. Unfortunately I renamed the examples after you forked the project. Before I can merge it, you need to rebase your fork.

@@ -54,14 +55,17 @@ class TodoList extends StatelessWidget {
icon: Icon(todo.done ? Icons.clear : Icons.check),
onPressed: () {
var newTodo = todo.copyWith(done: !todo.done);
Hive.box('todos')
.deleteAt(Hive.box('todos').values.toList().indexOf(todo));
Copy link
Member

Choose a reason for hiding this comment

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

Why did you replace delete(todo.id) with deleteAt(Hive.box('todos').values.toList().indexOf(todo))?

Deleting by key is much faster than your solution. You have to copy all todo items to a list, find the one to be deleted. Hive will then internally walk through all the values again to find the key of the todo item.

Copy link
Author

Choose a reason for hiding this comment

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

I tried it with the delete solution, it didn't work on my Pixel 3 XL emulator

Copy link
Member

@simc simc Aug 26, 2019

Choose a reason for hiding this comment

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

Strange. I'll test it on the emulator...

Edit: Does the web version work for you: https://leisim.github.io/hive/demos/todo

Copy link
Author

@SarcasticNoodle SarcasticNoodle Aug 26, 2019

Choose a reason for hiding this comment

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

Web version works for me, but when I restart the app on my emulator, it shows some weird behavior !
https://imgur.com/a/GPRXCQ2
I made sure, that the delete statement gets called.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, I'll look into it.

Hive.box('todos').put(newTodo.id, newTodo);
},
),
IconButton(
iconSize: 30,
icon: Icon(Icons.delete),
onPressed: () {
Hive.box('todos').delete(todo.id);
Hive.box('todos')
.deleteAt(Hive.box('todos').values.toList().indexOf(todo));
Copy link
Member

Choose a reason for hiding this comment

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

Here again...

Copy link
Author

Choose a reason for hiding this comment

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

I tried it with the delete solution, it didn't work on my Pixel 3 XL emulator

/// [openBox] function is loaded. While the box is not loaded a placeholder
/// [placeholderBuilder] is specified to inform the user. In the case of the
/// box failing to open an error widget [errorBuilder] is displayed.
class HiveBuilder extends StatelessWidget {
Copy link
Member

Choose a reason for hiding this comment

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

The HiveBuilder was initially part of hive_flutter because I found it useful. But it is not much more than a fancy FutureBuilder. Since handling Futures is a general Flutter problem and has nothing to do with Hive, I'm not sure if it should be included.

@simc
Copy link
Member

simc commented Sep 7, 2019

Unfortunately I could not merge this PR due to conflicts but all of your suggestions have been addressed. Thanks!

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