Skip to content

Preferences#15

Open
aleksandar-stefanovic wants to merge 4 commits intodonadigo:masterfrom
aleksandar-stefanovic:preferences
Open

Preferences#15
aleksandar-stefanovic wants to merge 4 commits intodonadigo:masterfrom
aleksandar-stefanovic:preferences

Conversation

@aleksandar-stefanovic
Copy link
Contributor

Added preferences dialog

The statement is redundant because the binding is already active (it works
even without explictly setting it))
Basically copied the one in Scratch.
@donadigo
Copy link
Owner

Thanks for contributing again! We don't need static instances in preferences dialog, since it's only used in one class, so you can just do in constructor of ToolBar class:
preferences_dialog = new PreferencesDialog ();

and then show it:
preferences_dialog.show_all ();

Creating methods like "get_general_box" isn't really needed and doesn't make sense here. The contents of get_general_box method should be in the constructor of the PreferencesDialog.

Although I like when someone contributes to my project, I don't want any pull requests submited now, just because there are still commits with huge diffs, the project is very young and often you will need to resolve conflicts before I can do a review, but if it's not a problem for you, feel free to sumbit them :)

@aleksandar-stefanovic
Copy link
Contributor Author

aleksandar-stefanovic commented Oct 29, 2016

I thought that there will be a complex layout in the future, so we could already split it into multiple methods, for readability (esp. considering possible multiple tabs).

All right, I just want to be helpful, if it's creating you a lot of work, I won't pull more requests, until the projects gets stable... regarding merging, it's not an issue for me, I don't think that I'm working on the important parts that would overlap, and if it happens, it usually isn't a big conflict...

...and removed redunant method from preferences dialog
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