Re: CComboBox losing selected value after performing multiple UpdateData
- From: Joseph M. Newcomer <newcomer@xxxxxxxxxxxx>
- Date: Thu, 16 Jun 2005 12:30:11 -0400
See below...
On 16 Jun 2005 07:38:01 -0700, "Anne" <anne1@xxxxxxxxxxx> wrote:
>Hi,
>I' m currently trying to bug fix a code written by someone else but I
>really don't understand what is going wrong...
>The code below is extracted from a small client application that sends
>data to a server and then display the server answer. For that I've got
>one combo box containing the data to send to the server : the user can
>choose to add a new one or re-use an old one by selecting it.
>
>The normal behaviour of my application should be :
>- If the user enter a new data in the combo then this data is added to
>the combo after the sending
>- If the user select data from the combo box, after the sending then
>the same data should still be selected
>
>The only problem is that the data is not selected anymore after the
>sending...
>
>void CConsoleDlg::OnSend()
>{
> UpdateData(TRUE);
****
This immediately screams "I LOVE WORKING ON OBSCURE BUGS AND SUFFERING NEEDLESSLY"
If you have to use UpdateData, you've already lost. Lose it instead
****
>
> bool firstTime = false;
>
> // --------------------------------------------------------------
> // Check if the value should be added to the combo
> // --------------------------------------------------------------
> // Note :
> // m_message : string variable used in order to store the combo value
> // this->m_strMessage : var holding the combo value
>
> if(m_message != this->m_strMessage)
> {
> m_message = this->m_strMessage;
> firstTime = true;
> }
>
****
Completely wrong approach. This is the sort of error that is made when you try using
UpdateData. Even the use of this-> shows a lot of confusion on how to use C++.
CString text;
m_combo.GetWindowText(text);
You should probably come up with a name a bit more informative than "m_combo".
*****
> // --------------------------------------------------------------
> // Send data to the server
> // --------------------------------------------------------------
*****
Presumably you are using CAsyncSocket. But how can you tell that it has actually been sent
correctly? What if the buffers were full and you have to delay because of WSA_EWOULDBLOCK?
The test is not nearly as simple as seeing if the sent request was sent "successfully";
you need to make sure it wasn't just delayed.
It is not clear what the firstTime boolean is doing. I can't see what purpose it serves. I
suspect that the purpose is probably the result of confusion about what is going on in a
send. Its very presence starts making me suspicious of this piece of code.
I get really suspicious when I see such an odd blend of code. For example, the presumption
that OnSend means that whatever you are going to send can be sent in its entirety.
Initiating the transfer from the combo box to the network by doing OnSend is also a
suspicious activity; there is no guarantee that when the OnSend arrives, which is
spontaneously, that the contents of the edit control will be complete, or even present.
The usual method is to extract the text you want to send, and initiate a send operation.
When the send completes, you have either sent the entire buffer or got a WSA_EWOULDBLOCK
notification, in which case you have to retain the part that was not sent to be
transmitted on the next OnSend notification. This peculiar piece of logic seems to suggest
that at any time, without warning, the contents of the edit control could be read, in
whatever state they are in (possibly in the middle of a change as the user is typing) and
that value may or may not be sent partially or entirely, possibly intermingled with the
partial send of a previous piece of information.
I think the reason you can't make sense of this code is that this code makes no sense. I
don't see anything wrong here that can't be solved by a complete rewrite of the logic of
sending data.
****
>
> // If sending was succesfull, Update edit control with sent data
> if (isent != SOCKET_ERROR)
> {
> CString s = GetCurrentTime();
> s = m_message;
> m_edtsend += s;
> m_edtsend += L"\r\n";
>
> if (firstTime)
> m_combo.InsertString(0,m_message);
>
> UpdateData(FALSE);
****
I have no idea what this could mean, because even if you WERE using UpdateData, this
call is completely pointless!
****
> }
> else
> {
> m_edtsend += "Error logged";
> m_edtsend += L"\r\n";
>
> if (firstTime)
> m_combo.InsertString(0,m_message);
>
> UpdateData(false);
****
Likewise pointless. Lose it
****
>
> // Try to send data again
> }
>
>
>
>}
>
>void CConsoleDlg::OnReceive()
>{
> // --------------------------------------------------------------
> // Handle server answer
> // --------------------------------------------------------------
>
> // --------------------------------------------------------------
> // Log server answer
> // --------------------------------------------------------------
> CString stin = GetCurrentTime();
> stin += strRecvd;
> m_edtrec += stin;
> m_edtrec += L"\r\n";
>
> UpdateData(FALSE);
****
What could this possibly mean? I see nothing here that could be updated. If there was an
intention to store the m_edtrec value, then the correct thing to do is to write
CString stin = GetCurrentTime();
stin += strRecvd;
c_Received.SetWindowText(stin);
Now there are many bugs here. For example, what does the \r\n do? Nothing. If this is a
static control, it isn't needed; if it is an edit control, it isn't needed. So what
purpose does it serve?
This code erroneously assumes that the entire reply is received. This is based on a common
misconception that send and receive actually send and receive an entire buffer, mistakenly
thought of as a "message". In fact, there is absolutely no correlation between the size of
data sent and the size of data received. The ONLY thing that is guaranteed is that all the
bytes sent are received, in order, with no duplicates. As to how MANY bytes are
transmitted or received, you have no idea how these correlate to a "message" unless you
have a way of figuring out where the message boundaries are. Your "receive" could receive
multiple messages, or only part of a message, and I see nothing here that indicates that
there is any attempt to make sense of the data received.
I'm not sure what bug you are fixing, but it appears that the fact that this code gives
the illusion of working at all is close to a miracle.
****
>
>}
>
>Inserting new data in the combo works fine but the strange behaviour
>happens when selecting a previously sent data, then with this piece of
>code the data currently selected is not kept after the sending. While I
>was testing this piece of code, I noticed that when commenting the
>UpdateData(FALSE) in the OnReceive function, then the selection was
>kept in my combo but of course nothing was written in my edit
>control...
>
****
Lose every trace of UpdateData. Create control member variables and manipulate the
controls you want in the way you want.
Given someone else wrote this code, I think you can safely assume they were largely
clueless about control management and using TCP/IP.
If you need to re-select text for re-sending, note that the data is preceded with a
timestamp. You would have to deal with stripping this off. You might want to keep a copy
of the string in the ItemData part so you don't have to reparse it.
****
>
>Any suggestions ?
>
>Anne
Joseph M. Newcomer [MVP]
email: newcomer@xxxxxxxxxxxx
Web: http://www.flounder.com
MVP Tips: http://www.flounder.com/mvp_tips.htm
.
- Follow-Ups:
- References:
- Prev by Date: resource edit bug
- Next by Date: Re: How to display the new item in report view of CListCtrl
- Previous by thread: Re: CComboBox losing selected value after performing multiple UpdateData
- Next by thread: Re: CComboBox losing selected value after performing multiple UpdateData
- Index(es):
Relevant Pages
|
