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

Migrate from Butterknife to View bindings #115

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

hades
Copy link

@hades hades commented May 17, 2024

I'm currently using the build off this branch, and it works fine, although I cannot guarantee that I've tested all the possible corner cases.

The changes are mostly mechanical, however in some cases (e.g. BaseDrawerActivity and BaseReportFragment) there were changes introduced in how subclasses interact with the base class.

Fixes #73

@@ -585,30 +575,14 @@ public void onClick(View v) {


class AccountViewHolder extends RecyclerView.ViewHolder implements PopupMenu.OnMenuItemClickListener {
@BindView(R.id.primary_text)
TextView accountName;
Copy link
Collaborator

Choose a reason for hiding this comment

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

final TextView accountName = binding.listItem.primaryText;

@BindView(R.id.input_budget_amount)
CalculatorEditText amountEditText;
@BindView(R.id.btn_remove_item)
ImageView removeItemBtn;
Copy link
Collaborator

Choose a reason for hiding this comment

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

final ImageView removeItemBtn = binding.btnRemoveItem;

View view = inflater.inflate(getLayoutResource(), container, false);
ButterKnife.bind(this, view);
View view = inflateView(inflater, container);
mSelectedValueTextView = view.findViewById(R.id.selected_chart_slice);
Copy link
Collaborator

Choose a reason for hiding this comment

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

= mBinding.selectedChartSlice

Copy link
Author

Choose a reason for hiding this comment

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

Same as BaseDrawerActivity

Comment on lines +92 to +95
public View inflateView(LayoutInflater inflater, ViewGroup container) {
mBinding = FragmentBarChartBinding.inflate(inflater, container, false);
return mBinding.getRoot();
Copy link
Collaborator

Choose a reason for hiding this comment

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

mChart = mBinding.barChart;

@hades
Copy link
Author

hades commented May 31, 2024

@pnemonic78 for the remaining changes, I'm not sure what you're requesting. Do you want to add a field to store some of the references to the views in the view binding for some reason?

@hades hades requested a review from pnemonic78 May 31, 2024 08:51
@pnemonic78
Copy link
Collaborator

@pnemonic78 for the remaining changes, I'm not sure what you're requesting. Do you want to add a field to store some of the references to the views in the view binding for some reason?

Yes. Keep the member fields, but instead of having ButterKnife assign them from the view, your assign them from the binding.

@hades
Copy link
Author

hades commented Jun 2, 2024

Understood. I'm going to disagree with your suggestion here: I don't think this improves code readability. For example, to me mBinding.barChart is actually more informative than mChart, as it tells me that it refers to a view with a given ID from the layout known from context.

Furthermore, in Android Studio you can use "Go To -> Implementation" to jump directly to the layout editor. Re-aliasing the view makes this a two-step process.

If in some cases the name of the binding is not clear, I would argue that the ID of the corresponding view should be renamed, instead of creating a local alias in code.

@pnemonic78
Copy link
Collaborator

Understood. I'm going to disagree with your suggestion here: I don't think this improves code readability. For example, to me mBinding.barChart is actually more informative than mChart, as it tells me that it refers to a view with a given ID from the layout known from context.

  1. having long names like mBinding.barChart make the code less readable.
  2. using the member field is akin to ViewHolder pattern (i.e. quicker access).
  3. can rename mChart to mBarChart

If in some cases the name of the binding is not clear, I would argue that the ID of the corresponding view should be renamed, instead of creating a local alias in code.

Definitely names in the XML should be better.

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.

migrate from butterknife to Jetpack view bindings
2 participants