Review Request 119905: Parley: Fix bugs: 338302 and 338417 title bar [modified] tag behavior is inconsistent.

[Inge Wallin]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/119905/#review65071
-----------------------------------------------------------


Patch looks good in general except for some bad naming of a function. It was good that you created a slot for FileNew int he view, that's where it belongs. I'm sure this can be pushed in one more iteration.


src/parleydocument.cpp
<https://git.reviewboard.kde.org/r/119905/#comment45473>

    You removed the call, but why not add a signal? That seems the natural thing to do.



src/parleymainwindow.h
<https://git.reviewboard.kde.org/r/119905/#comment45472>

    If the function updates connections, then it shouldn't be called update...Document(). I suggest documentUpdated() instead which indicates that that it should be called when the document is updated to take care of any bookkeeping (in this case updating the connections, but that is not relevant from the outside).



src/parleymainwindow.h
<https://git.reviewboard.kde.org/r/119905/#comment45471>

    Generally a comment like this should say what the function does, not how it does it.
    
    A better description would be something along the lines of "Creates a new document". Another comment is that in general in KDE names like slot...() are discouraged. Slots are normal functions that happen to be callable through signals in addition to the normal way. (But there are other examples of this case in Parley already so I'll let it slide.)



src/parleymainwindow.cpp
<https://git.reviewboard.kde.org/r/119905/#comment45474>

    This m_document->document() shows poor data hiding.  But that's historical so it's not your fault.
    
    But I am sure that there is a missing disconnect somewhere (not necessarily in this function). At least I couldn't find any. If a previous ParleyDocument is deleted there needs to be a disconnect of the signal.


- Inge Wallin


On Aug. 22, 2014, 6:23 p.m., Andreas Xavier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/119905/
> -----------------------------------------------------------
> 
> (Updated Aug. 22, 2014, 6:23 p.m.)
> 
> 
> Review request for KDE Edu.
> 
> 
> Bugs: 338302 and 338417
>     http://bugs.kde.org/show_bug.cgi?id=338302
>     http://bugs.kde.org/show_bug.cgi?id=338417
> 
> 
> Repository: parley
> 
> 
> Description
> -------
> 
> Symptoms:
> The [modified] in the title bar was inconsistent.
> 
> Problems:
> 1. The title bar was only connected to the dirty bit in the doc from the Editor.  Going directly welcome -> stats did not connect the title bar to the dirty bit.
> 2. Whenever a new document was created, new collection, welcome screen etc. the title bar was disconnected.
> 3. In several places setModified was called on the kvocdoc when either nothing changed or the kvocdoc should know better.
> 
> Solution:
> The title bar is ParleyMainWindow's title bar.  ParleyMainWindow also know when the document is updated.  I created a  updateParleyDocument so that ParleyMainWindow always knows when the doc changes and slotFileNew so that anyone can ask ParleyMainWindow to update its own title bar.
> 
> Not fixed: Parley still directly calls the setModified in kvocdoc when it starts practice.  kvocdoc needs to be fixed before that is removed.
> 
> 
> Diffs
> -----
> 
>   src/editor/editor.cpp 4fac862 
>   src/parleydocument.cpp 0ebeca1 
>   src/parleymainwindow.h 98a94ad 
>   src/parleymainwindow.cpp 9723ee1 
>   src/practice/practicemainwindow.cpp 3008501 
>   src/settings/documentproperties.cpp 02cbf7b 
>   src/settings/languageproperties.cpp e9b171f 
>   src/vocabulary/vocabularymodel.cpp abf0db9 
>   src/welcomescreen/welcomescreen.cpp d24f444 
> 
> Diff: https://git.reviewboard.kde.org/r/119905/diff/
> 
> 
> Testing
> -------
> 
> Clicked from welcomescreen -> stats -> practice -> exit stats
> Clicked from welcomscreen -> stats -> editor (edited) -> practice
> Clicked from welcomscreen -> stats -> editor (no edit) -> practice
> Created a new collection from welcome screen
> Changed doc properties from stats->File->Properties.
> 
> All of these cases worked correctly
> 
> 
> Thanks,
> 
> Andreas Xavier
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-edu/attachments/20140822/71dc79c8/attachment-0001.html>