-
Notifications
You must be signed in to change notification settings - Fork 249
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
Extract and re-use details screen starting code. #589 #591
base: master
Are you sure you want to change the base?
Conversation
* | ||
* @author Gabor Keszthelyi | ||
*/ | ||
public interface TaskDetailsUi |
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.
The interface looks quite generic. Why not just call it after something that can launch any activity? I mean, how would the same interface for the task editor?
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.
You're right, this can be more generic. How about something like this?:
interface Showable<C extends Context>
{
void show(C context);
}
It could represent a 'meaningful' UI of the app that can be shown. By 'meaningful' I mean that it represents a use-case, like 'details of the task', or like a 'permission request dialog'. Something that makes sense on its own and may be fired/shown independently of the place(s) where it is used.
The followings could be covered by this for example:
- opening a new screen by starting an
Activity
(C as Context) - showing a
DialogFragment
(C as Activity to call getFragmentManager()) - showing a
Toast
(C as Context) - showing a
SnackBar
(using findViewById(..content) (C as Activity) - showing a
Notification
(using activity.getSystemService(NotificationManager).notify(n)) (C as Context)
Fragment transactions would not be included. (Except for DialogFragment
)
Of course we wouldn't need to actually start to use it everywhere, I mentioned them just for the concept.
Let me know if you think that interface makes sense, I would update the code in this PR in that case.
5997f2c
to
980c57a
Compare
I've updated this with using that I also noticed however that there are 5 more places in the codebase where More specifically, we could create It probably makes sense to put this on hold, and discuss it before proceeding. |
No description provided.