Add a quick-call left pane to the main window #151
Conversation
|
To anyone else wondering what this is all about: This adds a vertical left pane, similar to the Buddy list, with the contents of the local address book. A single click will prefill the call input line with the contact's phone number (as if it had been selected from the Address book dialog), while a double click will automatically call that contact. I guess this could be useful for people who find themselves calling their contacts very frequently, by lowering the number of clicks (and mouse movements) required to do so. Otherwise, I can't really comment on the desirability or design of this feature, so I'll let other people chime in. Likewise, I can't say much about its UI, although I might have some remarks about the functionality of it all. What I can definitely say, however, is that this pull request is in no way, shape or form suitable for being merged in its current state. It's painfully obvious that its author did not bother looking at the file changes before submitting it. @RufusBarma, please click on the "Files changed" tab above and take the time to review the changes you are proposing. (I'll try to submit a review later on, but you should be able to easily spot most of the issues by yourself.) (Just to be clear: this is not meant to offend you or dismiss your work. It's always great to see people willing to pitch in and contribute to a project. It's just that acting carelessly can result in wasted time and effort for everyone involved.) |
fbriere
changed the title
| @@ -227,7 +227,7 @@ void GetAddressForm::deleteLocalAddress() | |||
| { | |||
| QModelIndexList sel = localListView->selectionModel()->selectedRows(); | |||
| if (sel.isEmpty()) | |||
| return; | |||
| return;https://github.com/LubosD/twinkle.git | |||
|
|
||
| void MphoneForm::selectLocalAddress() | ||
| { | ||
| qDebug()<<"Enter!"; |
There was a problem hiding this comment.
This does not belong in a release version.
|
|
||
| private: | ||
| void init(); | ||
| void destroy(); | ||
| bool shouldDisplayOSD(); | ||
| void updateOSD(); | ||
| QString lineSubstate2str( int line ); | ||
|
|
||
| AddressTableModel* m_model; |
There was a problem hiding this comment.
This is too generic a name for this purpose. m_model is used elsewhere to designate the single model of a specific form; MphoneForm, meanwhile, is much too big to immediately figure out which model we're talking about.
| @@ -29,6 +29,7 @@ public slots: | |||
| signals: | |||
| void address(const QString &, const QString &); | |||
| void address(const QString &); | |||
| void leavehere(); | |||
There was a problem hiding this comment.
What does this signal do? How does the name leavehere reflect that?
There was a problem hiding this comment.
I added this signal for detect potential changes in buddy list and refresh it in main form. Maybe there are better solution.
| void selectLocalAddress(); | ||
|
|
||
| private slots: | ||
| void on_viewAddressListAction_triggered(bool checked); |
There was a problem hiding this comment.
I should point out that auto-connect is not currently used anywhere throughout the code. I'm not saying it's a bad idea, but for consistency, maybe you should consider doing what the Buddy list does and adding the connect in Designer instead? (This is just my own personal opinion, though.)
There was a problem hiding this comment.
I undestand you. When I only start to do changes, I didn't know about the connect in Designer. I will fix it.
There was a problem hiding this comment.
In addition to the individual issues I raised, the question of white space is prevalent throughout this pull request. Please try to indent your new code so that it matches what you're inserting it into. (Although even the indentation of the current code is not always uniform, I admit.)
And please do not try to "fix" the indentation of existing code, unless you have a good reason. So many lines of this pull request are nothing but changes in white space.
| @@ -3049,6 +3072,16 @@ void MphoneForm::populateBuddyList() | |||
| buddyListView->expandAll(); | |||
| } | |||
|
|
|||
| void MphoneForm::populateAddressList() | |||
| { | |||
| m_model = new AddressTableModel(this, ab_local->get_address_list()); | |||
There was a problem hiding this comment.
Would it be possible to reuse the model from GetAddressForm instead? (As I remember, Qt allows two views to share the same model.) That way, you wouldn't need to populate/update it, as this would already be done for you.
|
@fbriere is there something more to do in the PR's or is it a problem with @LubosD that has no sufficient time to mantain/develop/review PR's /app. Maybe fork is needed here to release a new version of the program, right? |
I think that between the various issues I raised in my review above, the big red "Changes requested" indicator below, and my earlier comment stating that "this pull request is in no way, shape or form suitable for being merged in its current state", I think you're likely to find a few reasons why this PR has remained in its current state. (If you want to talk about the state of the project in general, I suggest that this topic be kept within #206. Thanks.) |
Add address book ui for quick call. One click - select. Two - call.
Some problem - in menu view translation for address book doesn't work.