Review Request 119467: Implement the main UI in QML (Kadixt patches 1/3)

Mon Aug 4 09:51:19 UTC 2014

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

Reviewboard ate my first round of comments and now they are showing up here after I've done them again, so appologies if some things are mentioned twice.

src/call-window.cpp
<https://git.reviewboard.kde.org/r/119467/#comment44393>

    This seems unused?

src/call-window.cpp
<https://git.reviewboard.kde.org/r/119467/#comment44409>

    Unused

src/call-window.cpp
<https://git.reviewboard.kde.org/r/119467/#comment44392>

    no spaces around "this"

src/call-window.cpp
<https://git.reviewboard.kde.org/r/119467/#comment44390>

    space around =

src/call-window.cpp
<https://git.reviewboard.kde.org/r/119467/#comment44391>

    the style in this block is all wrong

src/call-window.cpp
<https://git.reviewboard.kde.org/r/119467/#comment44420>

    What's with all the fullscreen messing about? Shouldn't it just show the dtmf no matter what the state is?

src/call-window.cpp
<https://git.reviewboard.kde.org/r/119467/#comment44410>

src/call-window.cpp
<https://git.reviewboard.kde.org/r/119467/#comment44421>

    This should probably be called "toggleFullScreen" as that is what it does actually

src/call-window.cpp
<https://git.reviewboard.kde.org/r/119467/#comment44394>

    codestyle (including the else below)

src/call-window.cpp
<https://git.reviewboard.kde.org/r/119467/#comment44406>

src/call-window.cpp
<https://git.reviewboard.kde.org/r/119467/#comment44411>

    This is rather strange, the action should be disabled in the UI rather than like this

src/call-window.cpp
<https://git.reviewboard.kde.org/r/119467/#comment44405>

    Are these really needed? The docs are not explicit on this after showFullScreen() has been called, but maybe just showNormal() would be enough?

src/call-window.cpp
<https://git.reviewboard.kde.org/r/119467/#comment44412>

    I'm wondering if this is really needed when "recovering" from "showFullScreen"...could also use some comment on why the window flags are being changed

src/call-window.cpp
<https://git.reviewboard.kde.org/r/119467/#comment44413>

    Instead of hiding, maybe we should force the qml window to be the active window?

src/dtmf-handler.h
<https://git.reviewboard.kde.org/r/119467/#comment44414>

    The forward declaration for DtmfWidget should be changed to DtmfQml and then the #include can be removed (/moved to the .cpp)

src/dtmf-qml.h
<https://git.reviewboard.kde.org/r/119467/#comment44415>

    All unused

src/qml-interface.h
<https://git.reviewboard.kde.org/r/119467/#comment44397>

    Unused

src/qml-interface.h
<https://git.reviewboard.kde.org/r/119467/#comment44416>

    From these I think only QDeclarativeView and QGst/ElementFactory are really needed, plus they are duplicated in the .cpp

src/qml-interface.h
<https://git.reviewboard.kde.org/r/119467/#comment44400>

src/qml-interface.h
<https://git.reviewboard.kde.org/r/119467/#comment44402>

src/qml-interface.h
<https://git.reviewboard.kde.org/r/119467/#comment44403>

    CallWindow* parent -> CallWindow *parent

src/qml-interface.cpp
<https://git.reviewboard.kde.org/r/119467/#comment44401>

    Unused

src/qml-interface.cpp
<https://git.reviewboard.kde.org/r/119467/#comment44417>

    Unused

src/qml-interface.cpp
<https://git.reviewboard.kde.org/r/119467/#comment44404>

    Remove

src/qml-interface.cpp
<https://git.reviewboard.kde.org/r/119467/#comment44418>

    Remove

src/qml/Main.qml
<https://git.reviewboard.kde.org/r/119467/#comment44407>

    This file has loooots of reds :S

src/qml/Main.qml
<https://git.reviewboard.kde.org/r/119467/#comment44408>

src/qml/Main.qml
<https://git.reviewboard.kde.org/r/119467/#comment44419>

    Magic numbers :S

- Martin Klapetek

On Aug. 2, 2014, 1:28 p.m., David Edmundson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/119467/
> -----------------------------------------------------------
> 
> (Updated Aug. 2, 2014, 1:28 p.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Repository: ktp-call-ui
> 
> 
> Description
> -------
> 
> Implement the main UI in QML (Kadixt patches 1/3)
> 
> 
> Diffs
> -----
> 
>   CMakeLists.txt 8c39e7c 
>   src/CMakeLists.txt 250aeb5 
>   src/call-window.h 07fd01e 
>   src/call-window.cpp c38112d 
>   src/call-window.ui 32c7dad 
>   src/callwindowui.rc 6c390b9 
>   src/dtmf-handler.h 91960dc 
>   src/dtmf-handler.cpp d8d7970 
>   src/dtmf-qml.h PRE-CREATION 
>   src/dtmf-qml.cpp PRE-CREATION 
>   src/dtmf-widget.h 9e1bc73 
>   src/dtmf-widget.cpp f3436b2 
>   src/dtmf-widget.ui 67f60b9 
>   src/qml-interface.h PRE-CREATION 
>   src/qml-interface.cpp PRE-CREATION 
>   src/qml/Main.qml PRE-CREATION 
>   src/qml/core/Button.qml PRE-CREATION 
>   src/qml/core/Dtmf.qml PRE-CREATION 
>   src/qml/core/DtmfButton.qml PRE-CREATION 
>   src/qml/core/Label.qml PRE-CREATION 
>   src/qml/core/Separator.qml PRE-CREATION 
>   src/qml/core/ToggleButton.qml PRE-CREATION 
>   src/qml/core/Toolbar.qml PRE-CREATION 
>   src/qml/core/Tooltip.qml PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/119467/diff/
> 
> 
> Testing
> -------
> 
> Rebased, patched and cherry-picked until I was red in the face.
> 
> Compiles, not tested beyond that.
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-telepathy/attachments/20140804/e3767c98/attachment-0001.html>