Refactored question() to take custom button names as params

[maluskid]

My name is Dominic and I'm a computer science student looking to get started in OSS. I've never contributed to any open source projects before, so please let me know if I'm doing anything incorrectly. I know that the CONTRIBUTING file mentions you should be a user of this program to contribute, but I saw an issue and figured it was something doable without understanding a ton of how backintime works. I wasn't sure where to introduce myself so I did it here as well as on the issue page.

Resolves #2483 - I edited the question() function in messagebox.py to work with the same functionality as before while also taking two additional optional string parameters to modify the text in the 'Yes' and 'No' boxes if desired. I also edited the relevant function call to cause the message box for importing/creating a configuration to read 'Import' and 'Configure' as per the issue. I ran and passed all the unittests in the qt/test directory. Let me know if there's any changes I can make/ways I can improve how I'm going about this. Thanks!

Screenshot:

I see now that I didn't adhere to the formatting for translatable strings. I've made the relevant changes.

[buhtz]

Hello Dominic,
On behalf of the team, thank you for your contribution and taking the time to improve Back In Time. We appreciate it.

Your PR looks fine. I would suggest some modifications.

• I would prefer not modify a standard function (messagebox.py::question()) just only to fit one exotic/special use case.
  • Please reset question() to its previous state.
  • Create the QMessageBox() directly in _import_config_from_backup().
• Adapt the question string in the message box so that it fits better to the buttons. Currently it is more a yes/no question.

Regards,
Christian

Best,
Christian

[maluskid]

Hey there I made the changes you mentioned. I noticed in GNOME desktop the import and create buttons were appearing in the opposite order that the prompt mentioned them in English so I reordered the prompt. I'd assume this won't work for all languages though.

Screenshot of new prompt:

[buhtz]

Hello,
this looks nice. I added on comment into the diff.

About the button ordering in GNOME. Nice that you consider this. The desktop environment is responsible for the ordering based on the button roles.

To preserve the ordering over all desktops I would suggest to use "ActionRole" for both buttons. The addButton() method returns a button object, you have to store. And you use clickedButton() to check which button the user choose. And to preselect on of the buttons use setDefaultButton().

btn_import = import_prompt.addButton(...)
btn_create = import_prompt.addButton(...)
import_prompt.setDefaultButton(???)
import_prompt.exec()
answer = import_prompt.clickedButton()
if answer == btn_import:
   ...

Which button we should pre-select? In older BIT versions on XFCE the "No" button is pre-selected. This would be "Create" in the new version. If you have no objects I would say to set "Create" as default.

The code becomes a bit complex. There is no high need, so feel free to decide yourself. If you want you can encapsulate this dialog in its own class (but in the same file "app.py"); e.g. ImportConfigQuestion or something else.

Fine work so far! 👍🌻

Christian

[maluskid]

Okay, understood. Thanks for the review and the suggestions! I'll look at the code again later this week and make some changes

[maluskid]

Okay, I cleaned up the code and made the changes you mentioned. It passes tests and works well on my computer. I noticed that the version changed, I believe that was caused by me running the make or configure scripts when testing my code? Not sure if you want me to leave it as is or not, let me know.

Screenshot:

[buhtz]

Thank you very much. This looks fine.

I nearly finished the blocking PR about the new mount subsystem. I am just waiting for test results from other contributors. You might want to jump in?

After this PR is merged, I can merge yours.

Feel free to add a changelog entry. e.g.

## Changed
- Improved import config dialog on first start (Dominic Maluski, @maluskid, [#2483](https://github.com/bit-team/backintime/issues/2483))

Regards,
Christian

[buhtz]

I would say this is a bit difficult to read for less experienced developers adn contributors.

Does this work and make sense?

Suggested change

	mark_main_profile_unsaved = not answer == btn_import
	mark_main_profile_unsaved = answer is btn_create

[maluskid]

I added those changes as well as a changelog entry! I'll look into testing your changes, but I'll have to go through setting up a VM so I don't brick my system testing things, which may take me a little while.

[buhtz]

Hi Dominic,

I'll look into testing your changes, but I'll have to go through setting up a VM so I don't brick my system testing things

VM is always good. But in this case you might simply use the --config <file> switch. In this case BIT will use a user defined path to the config file without touching your regular one.

Best,
Christian