Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix issue #468 'unit' is reset #587

Merged
merged 1 commit into from Jun 5, 2019
Merged

Conversation

SOLIDFred
Copy link
Contributor

@SOLIDFred SOLIDFred commented May 27, 2019

Hello, My apologizes I haven't contributed to anything outside of work before so I'm not sure please let me know if I miss some proper etiquette.

This issue resolves two problems, the first not mentioned (but related) to issue 468 was caused by code in welcome.py, mainly line 296

spin.setProperty("templateIndex", self.template[1].index(d))

The value of d was often the same for each tuple in template[1], usually 10, so every spinner was storing the first location where 10 was found in template[1].

If the spinner values were initially 1,2,10,10,10, and you changed them to be, 1,2,3,4,5, the "index(d)" would only see index 3 for the 10s, and it would run the connect function 3 times, updating index three with the value "5" on its last run resulting in 1,2,5,10,10.

Adding enumeration and using the actual index instead of deriving the index with the index() function resolves the problem completely.

Line 289 and 296 are all that are needed to resolve that issue.

And the second issue from 468 is that changes to the naming conventions aren't saved. This is because there simply doesn't seem to have been any intention to do so in the code. To resolve the issue I followed the same practice that had originally been done with the spinners, though for the QLineEdit values it sometimes resulted in a "None" being returned, which caused an error so I filtered None out with an if statement.

(I had to amend my commit as I had left some stuff in there that wasn't necessary, a print statement and an blank line)

@gedakc
Copy link
Collaborator

gedakc commented May 31, 2019

Thank you @NocturnalFred for your help to improve Manuskript. And congratulations on your first contribution to Free Software. :-)

From my review of the code this PR looks good. In my testing with this PR applied on Kubuntu 16.04 I confirmed that the new level "unit" is indeed saved when creating a project. Hence this PR does fix issue #468.

If there are no concerns then I plan to merge this PR with the develop branch in the next day or so.

Curtis

@gedakc gedakc added this to the 0.10.0 milestone May 31, 2019
@gedakc gedakc mentioned this pull request Jun 2, 2019
@gedakc
Copy link
Collaborator

gedakc commented Jun 5, 2019

@NocturnalFred before merging this request I performed one more test that exposed a problem with this PR.

To test I tried creating a project using the Empty fiction template and specifying six levels as shown in the screenshot below.

Manuskript-Create-Six-Level-Project

When I choose Create and provide a file name, then Manuskript remains stuck on the same screen and the process running Manuskript goes to 100% usage. EDIT 3: This appears to be an infinite loop. Just takes a long time.

EDIT 3: Please investigate to see if you can recreate this problem and if so then resolve the problem.

EDIT: Note this problem seems to appear in the master branch as well. The problem does not occur if three levels are specified.

EDIT 2: I think that a use case such as six levels is highly unusual.

@gedakc
Copy link
Collaborator

gedakc commented Jun 5, 2019

My bad. The test I tried to perform takes a long time to complete due to the huge amount of levels being created. I tried a smaller six level test and it did complete in a reasonable amount of time.

Manuskript-Create-Six-Level-Project-Test-2

I will proceed to merge this PR.

@gedakc gedakc merged commit 995eda1 into olivierkes:develop Jun 5, 2019
@SOLIDFred
Copy link
Contributor Author

@NocturnalFred before merging this request I performed one more test that exposed a problem with this PR.

To test I tried creating a project using the Empty fiction template and specifying six levels as shown in the screenshot below.

Manuskript-Create-Six-Level-Project

When I choose Create and provide a file name, then Manuskript remains stuck on the same screen and the process running Manuskript goes to 100% usage. EDIT 3: This appears to be an infinite loop. Just takes a long time.

EDIT 3: Please investigate to see if you can recreate this problem and if so then resolve the problem.

EDIT: Note this problem seems to appear in the master branch as well. The problem does not occur if three levels are specified.

EDIT 2: I think that a use case such as six levels is highly unusual.

Sorry for not seeing this earlier. I also noticed that many multipliers causes an issue. The first thing I did when I used manuskript was try to create something with many levels (I didn't know what I was doing) and it froze and I manually quit the program, and then I never tried doing that again. But there's no legitimate reason to do that afaik, I just wanted to do it because that's what the UI feels like it was telling me to do, and I didn't have any plans for a particular writing project in mind, I was just playing with manuskript.

There may be cause to limit, at least at welcome screen, the maximum possible levels and their multipliers. I don't know. I can't imagine, for example, any reason to have 100 levels of 100 somethings. But then again the user could simply realize after their first time making the mistake like I did not to do it again. Or perhaps some performance testing could be in order to see what a reasonable multiplier is, add a check and add some kind of warning system of "this many levels has been known to freeze the program" kind of thing. Maybe that's over kill.

@gedakc
Copy link
Collaborator

gedakc commented Jun 10, 2019

... there's no legitimate reason to do that afaik (create project with many lavels)

I agree. For now I think it's okay as there is no need that I can see to need even 10s of levels.

Thanks again for the patch. :-)

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.

None yet

2 participants