----------------------------------------------------------- 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 > >
_______________________________________________ KDE-Telepathy mailing list [email protected] https://mail.kde.org/mailman/listinfo/kde-telepathy
