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

#1 Fix global structure #8

Closed
wants to merge 3 commits into from
Closed

Conversation

marcelpinto
Copy link

I redesign the global structure of the project to use Builder Pattern.

Remove starting creation of all the steps view (was consuming a lot of time).

The view is created the first time is needed and reused in case is need. TODO remove view to release memory after view being used.

Fix bug with device lower v21 using ?attr/colorPrimary in progress_single_input_form.xml was making them crash, because was not able to find attr.

Remove the finish activity onFormCompleted, so it can be decided on the callback what to do.

Added Options example as well as modified the others.

@marcelpinto
Copy link
Author

I know is a big change of most everything, but I used a standard style and refactor a bit of the code.

The idea is really good, we will probably used in the next project, we will PR all the changes and you can decide if it's worth it to merge.

@heinrichreimer
Copy link
Owner

First of all thank you for committing on this library! 👍

Unfortuntely I have some requirements for merging your code:

  1. I won't be able to merge if your code is indented with two spaces. Please use the existing code style (1 tab) or Android studio's default style.
  2. Your code contains the removed OptionStep. Please remove it.
  3. Builders should be named CheckBoxStep.Builder(context).
  4. The Steps itself should have all attributes itself and shouldn't call getBuilder()

Until then I can't really understand the differences in code.

Sorry if this sounded offense to you :)

@marcelpinto
Copy link
Author

No problem. I understand that is a big change.

About the style is the one I've been using, but it can easily be changed.

The main benefit on using builder instead of constructors is that you can easily add new params and the structure of the code is more clear and it remove the repeated constructors that you had with repeated code.

To give you a clear example the native Android Dialogs is using this pattern in order to change parameters easily.

The other big change is the inflation of the views when you create the steps. I made a fast test adding 20 steps and it took more than 1s to create them.

Instead if you inflate the view when is needed is faster and less memory consuming.

About the point 3. I´m agree on that, it´s a more appropriated name.

The point 4 if you can suggest a better way... I´m not sure how to create the builder in the abstract class Step with all the attrs. This is why I did this method of getBuilder()

I will make the step 1 and 2 and you can decide if you want to merge.

Cheers.

@heinrichreimer
Copy link
Owner

Great!

Maybe I have to merge it by hand then but that shouldn't be a problem.

@heinrichreimer
Copy link
Owner

I currently haven't enough time to work on this project. Would you fix the pull request to match my suggestions, so I can update it to maven central? Thank you!

@marcelpinto
Copy link
Author

Sorry, I haven't had time to work on it, we won't use it so far, but If I find a moment to fix it I'll.

@thedumbtechguy
Copy link

I have an issue with removing ?attr/colorPrimary. Isn't this part of the support library?

@heinrichreimer
Copy link
Owner

@frostymarvelous Maybe open a new issue for that...

@heinrichreimer
Copy link
Owner

I really like the idea of a Builder pattern. I just want the attributes to stay in the Step class. We should use Builders only temporary for Step creation, not for accessing attributes.

@heinrichreimer
Copy link
Owner

Builder pattern is used in the next update.

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.

None yet

3 participants