OpenLP

Merge lp://qastaging/~suutari-olli/openlp/force-split into lp://qastaging/openlp

  1. force-split
  2. Merge into trunk
Proposed by Azaziah
Status: Superseded
Proposed branch: lp://qastaging/~suutari-olli/openlp/force-split
Merge into: lp://qastaging/openlp
Diff against target: 250 lines (+118/-11)
5 files modified
openlp/core/lib/renderer.py (+3/-3)
openlp/plugins/songs/forms/editsongform.py (+49/-0)
openlp/plugins/songs/forms/editversedialog.py (+6/-0)
openlp/plugins/songs/forms/editverseform.py (+44/-5)
tests/functional/openlp_plugins/songs/test_editverseform.py (+16/-3)
To merge this branch: bzr merge lp://qastaging/~suutari-olli/openlp/force-split
Reviewer Review Type Date Requested Status
Tim Bentley Needs Fixing
Review via email: mp+294228@code.qastaging.launchpad.net

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

Description of the change

- Added "Split" button for Song editor:
  This adds the currently selected Verses tag to
  the given position, thus creating a "Force split"
- Increased Optional Split limit from < 5 to < 51
- Increased test coverage

lp:~suutari-olli/openlp/force-split (revision 2665)
[←[1;32mSUCCESS←[1;m] https://ci.openlp.io/job/Branch-01-Pull/1543/
[←[1;32mSUCCESS←[1;m] https://ci.openlp.io/job/Branch-02-Functional-Tests/1454/
[←[1;32mSUCCESS←[1;m] https://ci.openlp.io/job/Branch-03-Interface-Tests/1392/
[←[1;32mSUCCESS←[1;m] https://ci.openlp.io/job/Branch-04a-Windows_Functional_Tests/1175/
[←[1;32mSUCCESS←[1;m] https://ci.openlp.io/job/Branch-04b-Windows_Interface_Tests/765/
[←[1;32mSUCCESS←[1;m] https://ci.openlp.io/job/Branch-05a-Code_Analysis/833/
[←[1;32mSUCCESS←[1;m] https://ci.openlp.io/job/Branch-05b-Test_Coverage/701/

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

Cannot test as the UI is not visible.
There will be more comments when it works

review: Needs Fixing
Revision history for this message
Azaziah (suutari-olli) wrote : Posted in a previous version of this proposal

Could you explain "Cannot test as the UI is not visible." ?

I replied to the other comments, thank you for the review.

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

You have and Insert button which is not visible - simple.

Revision history for this message
Azaziah (suutari-olli) wrote : Posted in a previous version of this proposal

> You have and Insert button which is not visible - simple.

Did you read my replies to code comments?

"What do you mean? The UI is the new "Split" button found in song editor,
in both "Edit all" and "Edit" modes."

This is visible for me, if it is not visible for you, then something has gone horribly wrong.

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

Ok found it now as the button is only visible on Edit All.

All the button does is add a ---[Verse X]--- tag which is the same as the splt button does except that the split button has more options.

Why is this request needed because it would seem we have the required functionality already.

Revision history for this message
Tomas Groth (tomasgroth) wrote : Posted in a previous version of this proposal

I think the idea is to force s split inside a verse - bot correct me if I'm wrong.
If that is the idea, then the button should also be available in single verse edit.

Revision history for this message
Azaziah (suutari-olli) wrote : Posted in a previous version of this proposal

> Ok found it now as the button is only visible on Edit All.
>
> All the button does is add a ---[Verse X]--- tag which is the same as the splt
> button does except that the split button has more options.
>
> Why is this request needed because it would seem we have the required
> functionality already.

----
> All the button does is add a ---[Verse X]--- tag which is the same as the splt
> button does except that the split button has more options.

Not sure if I understand this sentence, but:

With the "Split" button, the verse will be force splitted by using the verse tag.
This is easier than selecting the verse manually and then inserting it to the required position.
A lot of people are confusing the meaning of Optional split and try to perform force splits with it.
A lot of people have requested easier way to force split verses.

> Ok found it now as the button is only visible on Edit All.

Again: This is visible for me in: Edit, Edit all & Add,
are you sure this button is not visible for you as well?

Revision history for this message
Azaziah (suutari-olli) wrote : Posted in a previous version of this proposal

> I think the idea is to force s split inside a verse - bot correct me if I'm
> wrong.
> If that is the idea, then the button should also be available in single verse
> edit.

This button is available in single verse edit, as well it is available in "Add" verse.
I believe TRB143 may has missed it or something very weird is happening.

Revision history for this message
Tim Bentley (trb143) wrote :

OK now have this working a bit.
The insert only works on the Edit All screen and is not visible on Add or Edit.

Insert adds a ---[Verse-4]--- type tag

We already have split which does the same thing but with greater functionality as it allows one to choose what they want to add.

If Insert adds based on the thing which was last used and increments the count by one.

This is now very confusing.
Split needs to be enhanced with a new option to split the current verse type not add a 2nd implementation.

Looking at the defects they were old and could predate some of the current functionality like virtual split (only used if required).

review: Needs Fixing
Revision history for this message
Azaziah (suutari-olli) wrote :

> OK now have this working a bit.
> The insert only works on the Edit All screen and is not visible on Add or
> Edit.
>
> Insert adds a ---[Verse-4]--- type tag
>
> We already have split which does the same thing but with greater functionality
> as it allows one to choose what they want to add.
>
> If Insert adds based on the thing which was last used and increments the count
> by one.
>
> This is now very confusing.
> Split needs to be enhanced with a new option to split the current verse type
> not add a 2nd implementation.
>
>
> Looking at the defects they were old and could predate some of the current
> functionality like virtual split (only used if required).

I think you are confusing "Insert" and "Split"

"Insert" is old functionality, "Split" is new functionality.

"Insert" is available only in "Edit all", "Split" is also available in "Edit" & "Add"

"Insert" adds +1 to the current verse, as it is used to add more verses.
"Insert" allows you to change the verse type you want to insert.

"Split" duplicates the active verses tag, thus creating a force split

lp://qastaging/~suutari-olli/openlp/force-split updated
2666. By Azaziah

- Fixed the issue whre verse current number is not updated properly on edit single verse mode.

2667. By Azaziah

Merged trunk?

2668. By Azaziah

Merged trunk?

2669. By Azaziah

Reverted the perfectly functioning Verse split technigue and replaced it with insert [--].
This still needs to be turned into force split in renderer.py

2670. By Azaziah

- This is broken / Going to revert to older solution after this.

2671. By Azaziah

- Reverted to the working solution.

Unmerged revisions

2671. By Azaziah

- Reverted to the working solution.

2670. By Azaziah

- This is broken / Going to revert to older solution after this.

2669. By Azaziah

Reverted the perfectly functioning Verse split technigue and replaced it with insert [--].
This still needs to be turned into force split in renderer.py

2668. By Azaziah

Merged trunk?

2667. By Azaziah

Merged trunk?

2666. By Azaziah

- Fixed the issue whre verse current number is not updated properly on edit single verse mode.

2665. By Azaziah

Merged trunk (Noticed tests appear to be fixed)

2664. By Azaziah

- Improved this on Edit single verse (This now detects if the verse type is changed)

2663. By Azaziah

Added a test for def on_insert_button_clicked(self):

2662. By Azaziah

Fixed a test

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.