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

Show dialog if we fail to carry out the user's intent #189

Merged
merged 5 commits into from Feb 23, 2021

Conversation

marbetschar
Copy link
Member

Fixes #180. If we agree on this concept, we can expand it to task lists - which would save us some lines of code.

Copy link
Member

@danirabbit danirabbit left a comment

Choose a reason for hiding this comment

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

I don't think we should rely on the library error output here to provide feedback for users. The problem with abstracting error dialogs like this is that they're so generic that the feedback isn't useful for resolving the problem.

For example in add_task_async () the user is getting the exact same error for when we're unable to get the client as when we're unable to create an object. We could be providing much more accurate feedback than just that we failed to add a new task

@marbetschar
Copy link
Member Author

marbetschar commented Feb 16, 2021

Valid point. What about redefining error_received to something like this (not sure whether if its good design to have translations within TaskModel or not):

public signal void error_received (string title, string detail, Error? error = null);

Another way would be returning more detailed errors extending the already existing errordomain Tasks.TaskModelError - but that might be a bit overkill?

EDIT: After re-reading, I'm not sure if I completely understand your point. Redefining error_received is still an abstraction after all. Can you please elaborate how do you think the ideal solution should be like conceptionally?

@danirabbit
Copy link
Member

I'm not sure that this needs to be further abstracted at all. Granite.MessageDialog is already an abstraction

@marbetschar
Copy link
Member Author

@danrabbit how do you suggest to transport errors and their context from TaskModel to the UI? Should TaskModel itself be responsible of showing the MessageDialog?

Till now I thought of TaskModel as being part of the Model Layer and therefore UI agnostic … but maybe that's overthinking stuff…?

@danirabbit
Copy link
Member

@marbetschar public methods in the taskmodel should probably throw an error and then we can handle the error from wherever the method is called

@marbetschar
Copy link
Member Author

marbetschar commented Feb 20, 2021

@danrabbit thanks for your feedback! The involved part of the code seems much cleaner now: All task CRUD methods are async / non-blocking and potential errors are passed through and then displayed in the UI.

Although there might be better detail descriptions, I'm quite happy with the current state of work.

@danirabbit danirabbit merged commit babb79d into master Feb 23, 2021
@danirabbit danirabbit deleted the taskmodel-fail-loud branch February 23, 2021 20:14
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.

Don't fail silently when unable to write
2 participants