Restore progress bar functionality #654
Conversation
A previous fix (5f9ea3) inadvertently broke the progress bar by converting to the wrong data type. (See issue olivierkes#561 / PR olivierkes#609). While checking the code I realized the problem occurred primarily because we weren't checking the validity of the values closer to the source. Doing so alleviates the need to check elsewhere. In the hope of inspiring a more systematic approach, a new uiParse() utility function has been added to curb the further growth of toXxx() functions that exist solely to validate user input. There is no doubt room for improvement, both on the end of the new uiParse() function as well as the spot where it is used. Ideally, the data that comes out of the model should already be 'safe', but since this is a bugfix for a bugfix I want to keep waves to a minimum. This commit fixes issue olivierkes#652.
|
Great work @worstje. This PR both restores the progress bar, and prevents the crash when setting a goal in the Outline pane. A big improvement over my initial attempt. I think we are ready for the 0.10.0 release after I merge the outstanding PR's for the 0.10.0 milestone. With the realization that we will never have a bug free release, is there anything else that you think must be included for 0.10.0? |
|
I know you asked me to look at the UI stuff regarding showing revision deprecation in the UI, but I sort of avoided that one by keeping myself busy in the real world and because I haven't done UI stuff before. (I keep postponing dipping my toes into that; I think I may have a bit of Qt phobia at this point. 😁 ) The smart remove thing was completely scrapped, but I don't know if that crash bug itself was ever fixed? If not, I feel an easy crash bug like that should at the very least be handled by not allowing the user to select a zero or other invalid value to begin with. [Edit: Should I tackle this still, and to what degree? I should have a few hours left on my evening tonight.] Besides that, although it is not technically a release thing.. there is the matter of semaphoreci builds always failing. I realize it takes @olivierkes to fix that stuff, but can we finally get it looked at for the sake of not seeing red crosses on every PR / commit anymore? |
The bug still exists, but once we disable "keep revisions" by default, then a user would have to first enable the keep revisions feature and then try to set a zero revision value for the crash to occur.
This bug has proven to be outside our current skill set to address properly. I have looked at it myself, but do not know how to configure the Qt GUI to prevent the zero "0" value being selected. If you can fix it easily then we can certainly wait a few days. 'Just let me know if you want to tackle it. As far as I know the bug has existed since the automatic revision saving feature was added. Personally I think our time is better spent fixing other more important areas of Manuskript.
I have sent an email to @olivierkes requesting that SemaphoreCI either be fixed or removed. Other than that my hands are tied because I do not appear to have access to change SemaphoreCI. |
|
I'll take some time to look into it now. If it is an easy fix, yay. And if it isn't, it is still a learning opportunity since I need to get my hands wet with UI stuff at some point, and this might as well be the one. (It is a whole lot more managable than my other idyllic dreams of splitting up all project configuration settings into their own area...) |
Okay. In the meantime I will proceed to merge the outstanding 0.10.0 milestone items. |
|
@gedakc Is there any way to adjust the contribution page on the website to make contributing as a coder more prominent? It mentions the program needing testers, but what we really need are people who know how to code and aren't afraid to help out. And since I am mentioning community stuff.. can we perhaps make that community Discord server a little bit more prominent? It is a lower barrier of entry / participation for people who aren't quite coders and are scared away by the entire 'Github' thing. (To be honest, I would love to have a slightly more direct line of communication with you for smaller issues, and Discord is great for that. I hate polluting PRs and issues with tons of random conversation regarding the Manuskript as a whole.) |
|
@worstje, I've updated the contribution page per your suggestion. The other questions on communications deserve a considered response. I will create a new issue after I've had time to give some serious thought to a response. Meanwhile I will merge this PR with the develop branch. |
|
That's totally fine. 👍 I realize there is a probably a need and/or desire to keep some distance to not be personally overwhelmed with opensource maintenance matters, as well as the ideal (from a project perspective) to provide transparency by encouraging things to be discussed on a public forum. So I appreciate you taking it into consideration. |
A previous fix (5f9ea3) inadvertently broke the progress bar by
converting to the wrong data type. (See issue #561 / PR #609).
While checking the code I realized the problem occurred primarily
because we weren't checking the validity of the values closer to the
source. Doing so alleviates the need to check elsewhere.
In the hope of inspiring a more systematic approach, a new uiParse()
utility function has been added to curb the further growth of toXxx()
functions that exist solely to validate user input.
There is no doubt room for improvement, both on the end of the new
uiParse() function as well as the spot where it is used. Ideally, the
data that comes out of the model should already be 'safe', but since
this is a bugfix for a bugfix I want to keep waves to a minimum.
This commit fixes issue #652.