Rearrange widgets in settings window and refactor code related

[SilverBut]

Currently, some files under idarling/interface are kind of too complex and should be refactored.

Also, the settings window don't have a common save/cancel/reset button, which is kind of awkward for users.

I would refactor those code and make a change to the widgets.

[SilverBut]

And if everything goes right, this issue, along with issue #44 and #45, will have a related pull request after less than 72 hours. Just assign those issues directly to me.

[NeatMonster]

I have a question for everyone involved: do you think we should keep everything inside the same window? If not, should we add our menu into IDA? I do like the idea of using only the widget though. :-/

[SilverBut]

Actually, there are not so many differences...

Currently, we are using a TabWidget with many pages, and for each page, a QWidget is created to put many controls. When closing the window, settings in the QWidget will be saved.

If we use a menu, we can just remove the TabWidget and add a warper function like show_config_dialog(CfgSettingsWidget), which would create a dialog, put in the related widget, and implemented some save/load code. Or we can also have a dir interface/settings/*.py to use a base class to do all those things above.

The latter solution seems to have a much more clean code, but that programming model can still be applied to the TabWidget. Instead, I would like to keep the current style, since I can modify general settings and server settings in the same window, instead of finding it again in the drop-down menu.

[NeatMonster]

@SilverBut Thank you for your input. I'm on the "keeping everything as is" side too. @patateqbool?

[patateqbool]

Same here, I prefer the current style. Thanks for your input @SilverBut.

[NeatMonster]

I have made some changes related to this issue in commits 7dad355 and 51b6a97.

[SilverBut]

I have made some changes related to this issue in commits 7dad355 and 51b6a97.

Noticed that. I'm about 30% of merging those code...real world stuffs is kind of busier than I was thought.

[SilverBut]

Besides I recommend we split some commits.... such as in comit 7dad355, it says "Save/Reset/Cancel Buttons" are added, but after reviewed the code you might noticed:

• Default config generator is changed, with default color generater merged
• Refactored code for setting colors to use methods from Qt
• Fixed bug of loading wrong logging level
• Adding settings reset/cancel mechanism
• Form structure changed, and using a widget as a container

They should be pushed as seperate commits, or be described in detail in the commit comments, instead of making maintainers to see the code line-by-line.

Besides, maybe we can use a CONTRIBUTING file to tell future committers how to do code style check or other stuffs.

[SilverBut]

Commits generated by black and flake8 should also be committed separately.

[NeatMonster]

Besides I recommend we split some commits....

You're absolutely right about that. I've always found it difficult to fragment my changes into smaller commits. While fixing a bug, I usually end up finding some new bugs, that I fix immediately. I probably shouldn't do that. I'll try to improve on that, and I think I can use PyCharm to select what changes to commit each time.

Besides, maybe we can use a CONTRIBUTING file to tell future committers how to do code style check or other stuffs.

Sure, I haven't taken the time to do it, but I definitively will!

Commits generated by black and flake8 should also be committed separately.

What do you mean by that? I made a single commit when switching to black, and switching to flake8.

[SilverBut]

What do you mean by that? I made a single commit when switching to black, and switching to flake8.

Yeah I know. I just mean that in case of new contributors break the code style, they should use one single commit for fixing it (like this) instead of add a code style commit everytime they added a real commit to the PR.

By the way, PR #52 is added to fix #51 and #46. You can review and merge them if you need.

[NeatMonster]

If we're going to separate tabs into multiple files, shouldn't we make the reset button reset only the current tab (as opposed to resetting all tabs as we currently do)?

[SilverBut]

shouldn't we make the reset button reset only the current tab (as opposed to resetting all tabs as we currently do)?

That is a good idea. We may implement getter & setter methods for each widget so they can be easily resetted by providing a None argument.

[NeatMonster]

I believe only splitting the code into smaller files (#38) is still missing. Closing this issue.