Merge lp://qastaging/~thelinuxguy/openlp/change-dropdown-to-checkbox into lp://qastaging/openlp

Proposed by Simon Hanna
Status: Merged
Approved by: Tim Bentley
Approved revision: 2598
Merged at revision: 2662
Proposed branch: lp://qastaging/~thelinuxguy/openlp/change-dropdown-to-checkbox
Merge into: lp://qastaging/openlp
Diff against target: 133 lines (+34/-29)
3 files modified
openlp/core/ui/plugindialog.py (+4/-12)
openlp/core/ui/pluginform.py (+15/-17)
tests/functional/openlp_core_common/test_actions.py (+15/-0)
To merge this branch: bzr merge lp://qastaging/~thelinuxguy/openlp/change-dropdown-to-checkbox
Reviewer Review Type Date Requested Status
Tim Bentley Approve
Raoul Snyman Approve
Review via email: mp+294957@code.qastaging.launchpad.net

This proposal supersedes a proposal from 2016-05-17.

Description of the change

* Change the Combobox used for the state of plugins to a checkbox.
* Do not show the plugins version numbers as they provide no additional information
* Show the plugin details (about text) even if the plugin is disabled

To post a comment you must log in.
Revision history for this message
Tim Bentley (trb143) wrote : Posted in a previous version of this proposal

Disable means the plugin cannot work due to missing dependencies like Impress or Powerpoint.
Inactive means that the plugin is not to be used i.e you do not want it running in most cases this would be remote.

Looking at the code the combo box needs to support three states not two so this change is not valid as a check box can only have 2 states so this is not a valid change.

The change to about looks valid but why only one plugin why not all of them?

Wew have a test, good but it seems a bit lite!

review: Disapprove
Revision history for this message
Simon Hanna (thelinuxguy) wrote : Posted in a previous version of this proposal

But the third state doesn't allow the user to change anything.
I guess the third state could be emulated by either disabling the checkbox or replacing it with a label saying it's missing a dependency.

About the change to about:
I only changed about because I was looking for something to test and came across this function.
I can go through the others and change them as well...

about the lite test:
I know it's lite. But I'm not sure what else to test for the about function

Revision history for this message
Tim Bentley (trb143) wrote : Posted in a previous version of this proposal

No the third state is there because the machine cannot support the plugin, just disabling the check box is not a good indication ofthat state.

Revision history for this message
Simon Hanna (thelinuxguy) wrote : Posted in a previous version of this proposal

and replacing it with a label?

Revision history for this message
Tim Bentley (trb143) wrote : Posted in a previous version of this proposal

Not in my opinion.

Revision history for this message
Simon Hanna (thelinuxguy) wrote : Posted in a previous version of this proposal
Revision history for this message
Simon Hanna (thelinuxguy) wrote : Posted in a previous version of this proposal

I just checked, when a plugin is disabled (you can just replace the songs.sqlite with a normal file)
The whole entry becomes disabled. So there really is no reason for the dropdown. Currently the branch throws an Exception when a plugin is disabled, but that can be fixed. What do you think?

Revision history for this message
Raoul Snyman (raoul-snyman) wrote : Posted in a previous version of this proposal

Hey Simon, do you want to update your branch and re-propose?

Revision history for this message
Raoul Snyman (raoul-snyman) wrote : Posted in a previous version of this proposal

(I mean, merge from trunk)

Revision history for this message
Tim Bentley (trb143) wrote : Posted in a previous version of this proposal

Great code but we do need a test as all merges require tests.

Sorry.

review: Needs Fixing
2598. By Simon Hanna

Add comments to test

Revision history for this message
Raoul Snyman (raoul-snyman) :
review: Approve
Revision history for this message
Tim Bentley (trb143) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.