n4af / TR4W

TRLOG 4 Windows free amateur radio logging application
GNU General Public License v3.0
19 stars 6 forks source link

Lock the bandmap window while updating to prevent flashing #688

Open ny4i opened 10 months ago

ny4i commented 10 months ago

During frequent updates, the bandmap flashes from updating so frequently. One possible solution is to lock the window with the Win32 LockWindowUpdate function.

I do not recall if I tried the this before.

Using this ticket to track it.

https://learn.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-lockwindowupdate

ny4i commented 10 months ago

The LockWindowUpdates documentation suggested to send the message WM_SETREDRAW instead. I tried that and while it made some difference, the BM still flashes.

One thing I learned is it appears the telnet code only uses a thread for the connection. I thought I must be reading that wrong, but when I added a sleep (250) to sleep 250 milliseconds, the whole UI slowed down. My thought was to limit how often the update occur but the telnet thread should be in a thread instead.

Another thought is to set a TTimer to fire every 250 milliseconds and do the update.

Even without adding full threading for telnet (which really should happen), that can be tested by setting BMDELAY to 250.

n4af commented 10 months ago

alas- the spot must sync to the display. else, clicking on a b.m. entry displays the wrong call entry/frequency.. likewise, delaying spot processing results in lost spots.

On Fri, Sep 1, 2023 at 12:14 PM Tom Schaefer @.***> wrote:

The LockWindowUpdaes suggested to send the message WM_SETREDRAW https://learn.microsoft.com/en-us/windows/desktop/gdi/wm-setredraw instead. I tried that and while it made some difference, the BM still flashes.

One thing I learned is it appears the telnet code only uses a thread for the connection. I thought I must be reading that wrong, but when I added a sleep (250) to sleep 250 milliseconds, the whole UI slowed down. My thought was to limit how often the update updates but the telnet thread should be in a thread instead.

Another thought is to set a TTimer to fire every 250 milliseconds and do the update.

Even without adding full threading for telnet (which really should happen), that can be tested by setting BMDELAY to 250.

— Reply to this email directly, view it on GitHub https://github.com/n4af/TR4W/issues/688#issuecomment-1702996550, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABVLCUHBKQWCJKC55AB7UWDXYICVNANCNFSM6AAAAAA4HYZBPY . You are receiving this because you are subscribed to this thread.Message ID: @.***>

ny4i commented 10 months ago

Normally, the way to handle that is to store an object with the list box entry which is the spot data. That way clicking on any spot will always give the correct info. I’m not sure how to do that yet with Win32 but I’ll look. TomPrincipal Solutions ArchitectBetter Software Solutions, Inc. 727-437-2771On Sep 1, 2023, at 12:59 PM, Howie Hoyt @.***> wrote: alas-

the spot must sync to the display. else, clicking on a b.m. entry displays

the wrong call entry/frequency..

likewise, delaying spot processing results in lost spots.

On Fri, Sep 1, 2023 at 12:14 PM Tom Schaefer @.***>

wrote:

The LockWindowUpdaes suggested to send the message WM_SETREDRAW

https://learn.microsoft.com/en-us/windows/desktop/gdi/wm-setredraw

instead. I tried that and while it made some difference, the BM still

flashes.

One thing I learned is it appears the telnet code only uses a thread for

the connection. I thought I must be reading that wrong, but when I added a

sleep (250) to sleep 250 milliseconds, the whole UI slowed down. My thought

was to limit how often the update updates but the telnet thread should be

in a thread instead.

Another thought is to set a TTimer to fire every 250 milliseconds and do

the update.

Even without adding full threading for telnet (which really should

happen), that can be tested by setting BMDELAY to 250.

Reply to this email directly, view it on GitHub

https://github.com/n4af/TR4W/issues/688#issuecomment-1702996550, or

unsubscribe

https://github.com/notifications/unsubscribe-auth/ABVLCUHBKQWCJKC55AB7UWDXYICVNANCNFSM6AAAAAA4HYZBPY

.

You are receiving this because you are subscribed to this thread.Message

ID: @.***>

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: @.***>

ny4i commented 10 months ago

The above is wrong. The Telnet code is definitely in a thread but the part where it calls DisplayBandMap is not so more checking is required.

ny4i commented 10 months ago

I'm quite confused. I am in the state of "How could this possibly work"? When a spot is double-clicked, I see the code get an index by sending the message, LB_GETITEMDATA. But I cannot find anything related to the spot that does the corresponding LB_SETITEMDATA. Typically you would set an index then store that with the listbox entry. When accessed, you get the index (integer) back and use it to index a list (FLIST in this case).

The other thing that is missing considering this is threaded code is any sort of locking around the index creation. The pattern should be grab a lock (TCriticalSection), assign the add the spot to the SpotList object, add the value to the bandmap list box including the LB_SETITEMDATA call), then free the lock. In threaded code (which Telnet is, this is hit or miss.

I do see some code in uTelnet that uses LB_SETITEMDATA but that is for the entry in the telnet window. It might be possible that the whole telnet record is passed to the spotLIst but that seems odd.

Looking at this code (and it is definitely a big pile of spaghetti), I would say a completely new object should be rewritten to manage the spots. The SpotsList object is the right idea but this code is really rough.

I of course could just not be understanding it fully yet but I am not following parts of it.

A simpler thing might be to just put a criticalsection around where the spot is created in the SpotsList and anywhere the SpotsList is modified (Add, Delete, etc).

n4af commented 10 months ago

Hi Tom -

it is triggered by Uspots proc TDXSpotslListSetCursor FCurrentCursorFreq := GetBMSelItemData; Returns the entry # in the BM.

On Sun, Sep 3, 2023 at 6:29 PM Tom Schaefer @.***> wrote:

I'm quite confused. I am in the state of How could this possibly work reliably? When a spot is double-clicked, I see the code get an index by sending the message, LB_GETITEMDATA. But I cannot find anything related to the spot that does the corresponding LB_SETITEMDATA. Typically you would set an index then store that with the listbox entry. When accessed, you get the index (integer) back and use it to index a list (FLIST in this case).

The other thing that is missing considering this is threaded code is any sort of locking around the index creation. The pattern should be grab a lock (TCriticalSection), assign the add the spot to the SpotList object, add the value to the bandmap list box including the LB_SETITEMDATA call), then free the lock. In threaded code (which Telnet is, this is hit or miss.

I do see some code in uTelnet that uses LB_SETITEMDATA but that is for the entry in the telnet window. It might be possible that the whole telnet record is passed to the spotLIst but that seems odd.

Looking at this code (and it is definitely a big pile of spaghetti), I would say a completely new object should be rewritten to manage the spots. The SpotsList object is the right idea but this code is really rough.

I of course could just not be understanding it fully yet but I am not following parts of it.

A simpler thing might be to just put a criticalsection around where the spot is created in the SpotsList and anywhere the SpotsList is modified (Add, Delete, etc).

— Reply to this email directly, view it on GitHub https://github.com/n4af/TR4W/issues/688#issuecomment-1704421962, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABVLCUDLI65LRYMSPQL6DBLXYUAC3ANCNFSM6AAAAAA4HYZBPY . You are receiving this because you commented.Message ID: @.***>

ny4i commented 10 months ago

What I can’t find is the corresponding SetItemDataPrincipal Solutions ArchitectBetter Software Solutions, Inc. 727-437-2771On Sep 4, 2023, at 7:52 AM, Howie Hoyt @.***> wrote: Hi Tom -

it is triggered by Uspots proc TDXSpotslListSetCursor

FCurrentCursorFreq := GetBMSelItemData;

Returns the entry # in the BM.

On Sun, Sep 3, 2023 at 6:29 PM Tom Schaefer @.***>

wrote:

I'm quite confused. I am in the state of How could this possibly work

reliably? When a spot is double-clicked, I see the code get an index by

sending the message, LB_GETITEMDATA. But I cannot find anything related to

the spot that does the corresponding LB_SETITEMDATA. Typically you would

set an index then store that with the listbox entry. When accessed, you get

the index (integer) back and use it to index a list (FLIST in this case).

The other thing that is missing considering this is threaded code is any

sort of locking around the index creation. The pattern should be grab a

lock (TCriticalSection), assign the add the spot to the SpotList object,

add the value to the bandmap list box including the LB_SETITEMDATA call),

then free the lock. In threaded code (which Telnet is, this is hit or miss.

I do see some code in uTelnet that uses LB_SETITEMDATA but that is for the

entry in the telnet window. It might be possible that the whole telnet

record is passed to the spotLIst but that seems odd.

Looking at this code (and it is definitely a big pile of spaghetti), I

would say a completely new object should be rewritten to manage the spots.

The SpotsList object is the right idea but this code is really rough.

I of course could just not be understanding it fully yet but I am not

following parts of it.

A simpler thing might be to just put a criticalsection around where the

spot is created in the SpotsList and anywhere the SpotsList is modified

(Add, Delete, etc).

Reply to this email directly, view it on GitHub

https://github.com/n4af/TR4W/issues/688#issuecomment-1704421962, or

unsubscribe

https://github.com/notifications/unsubscribe-auth/ABVLCUDLI65LRYMSPQL6DBLXYUAC3ANCNFSM6AAAAAA4HYZBPY

.

You are receiving this because you commented.Message ID:

@.***>

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: @.***>

n4af commented 10 months ago

The call stack looks like: LBN_Selchange SpotsList.SetCursor GetBMSelItemData <<< line 536 uSpots FCurrentCursorFreq :=

On Mon, Sep 4, 2023 at 9:43 AM Tom Schaefer @.***> wrote:

What I can’t find is the corresponding SetItemDataPrincipal Solutions ArchitectBetter Software Solutions, Inc. 727-437-2771On Sep 4, 2023, at 7:52 AM, Howie Hoyt @.***> wrote: Hi Tom -

it is triggered by Uspots proc TDXSpotslListSetCursor

FCurrentCursorFreq := GetBMSelItemData;

Returns the entry # in the BM.

On Sun, Sep 3, 2023 at 6:29 PM Tom Schaefer @.***>

wrote:

I'm quite confused. I am in the state of How could this possibly work

reliably? When a spot is double-clicked, I see the code get an index by

sending the message, LB_GETITEMDATA. But I cannot find anything related to

the spot that does the corresponding LB_SETITEMDATA. Typically you would

set an index then store that with the listbox entry. When accessed, you get

the index (integer) back and use it to index a list (FLIST in this case).

The other thing that is missing considering this is threaded code is any

sort of locking around the index creation. The pattern should be grab a

lock (TCriticalSection), assign the add the spot to the SpotList object,

add the value to the bandmap list box including the LB_SETITEMDATA call),

then free the lock. In threaded code (which Telnet is, this is hit or miss.

I do see some code in uTelnet that uses LB_SETITEMDATA but that is for the

entry in the telnet window. It might be possible that the whole telnet

record is passed to the spotLIst but that seems odd.

Looking at this code (and it is definitely a big pile of spaghetti), I

would say a completely new object should be rewritten to manage the spots.

The SpotsList object is the right idea but this code is really rough.

I of course could just not be understanding it fully yet but I am not

following parts of it.

A simpler thing might be to just put a criticalsection around where the

spot is created in the SpotsList and anywhere the SpotsList is modified

(Add, Delete, etc).

Reply to this email directly, view it on GitHub

https://github.com/n4af/TR4W/issues/688#issuecomment-1704421962, or

unsubscribe

< https://github.com/notifications/unsubscribe-auth/ABVLCUDLI65LRYMSPQL6DBLXYUAC3ANCNFSM6AAAAAA4HYZBPY>

.

You are receiving this because you commented.Message ID:

@.***>

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: @.***>

— Reply to this email directly, view it on GitHub https://github.com/n4af/TR4W/issues/688#issuecomment-1705300663, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABVLCUHRKVNWW4YTSLCMQNLXYXLHXANCNFSM6AAAAAA4HYZBPY . You are receiving this because you commented.Message ID: @.***>

ny4i commented 10 months ago

Yes but I’m order for the index to come from GetBMSelItemData, there has to be a corresponding SetItemData. I don’t see that. TomPrincipal Solutions ArchitectBetter Software Solutions, Inc. 727-437-2771On Sep 4, 2023, at 10:18 AM, Howie Hoyt @.***> wrote: The call stack looks like:

LBN_Selchange

SpotsList.SetCursor

GetBMSelItemData <<< line 536 uSpots

FCurrentCursorFreq :=

On Mon, Sep 4, 2023 at 9:43 AM Tom Schaefer @.***>

wrote:

What I can’t find is the corresponding SetItemDataPrincipal Solutions

ArchitectBetter Software Solutions, Inc. 727-437-2771On Sep 4, 2023, at

7:52 AM, Howie Hoyt @.***> wrote:

Hi Tom -

it is triggered by Uspots proc TDXSpotslListSetCursor

FCurrentCursorFreq := GetBMSelItemData;

Returns the entry # in the BM.

On Sun, Sep 3, 2023 at 6:29 PM Tom Schaefer @.***>

wrote:

I'm quite confused. I am in the state of How could this possibly work

reliably? When a spot is double-clicked, I see the code get an index by

sending the message, LB_GETITEMDATA. But I cannot find anything related

to

the spot that does the corresponding LB_SETITEMDATA. Typically you would

set an index then store that with the listbox entry. When accessed, you

get

the index (integer) back and use it to index a list (FLIST in this

case).

The other thing that is missing considering this is threaded code is any

sort of locking around the index creation. The pattern should be grab a

lock (TCriticalSection), assign the add the spot to the SpotList object,

add the value to the bandmap list box including the LB_SETITEMDATA

call),

then free the lock. In threaded code (which Telnet is, this is hit or

miss.

I do see some code in uTelnet that uses LB_SETITEMDATA but that is for

the

entry in the telnet window. It might be possible that the whole telnet

record is passed to the spotLIst but that seems odd.

Looking at this code (and it is definitely a big pile of spaghetti), I

would say a completely new object should be rewritten to manage the

spots.

The SpotsList object is the right idea but this code is really rough.

I of course could just not be understanding it fully yet but I am not

following parts of it.

A simpler thing might be to just put a criticalsection around where the

spot is created in the SpotsList and anywhere the SpotsList is modified

(Add, Delete, etc).

Reply to this email directly, view it on GitHub

https://github.com/n4af/TR4W/issues/688#issuecomment-1704421962, or

unsubscribe

<

https://github.com/notifications/unsubscribe-auth/ABVLCUDLI65LRYMSPQL6DBLXYUAC3ANCNFSM6AAAAAA4HYZBPY>

.

You are receiving this because you commented.Message ID:

@.***>

—Reply to this email directly, view it on GitHub, or unsubscribe.You are

receiving this because you authored the thread.Message ID: @.***>

Reply to this email directly, view it on GitHub

https://github.com/n4af/TR4W/issues/688#issuecomment-1705300663, or

unsubscribe

https://github.com/notifications/unsubscribe-auth/ABVLCUHRKVNWW4YTSLCMQNLXYXLHXANCNFSM6AAAAAA4HYZBPY

.

You are receiving this because you commented.Message ID:

@.***>

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: @.***>

ny4i commented 10 months ago

I will add sone debug messages around the AddSpot to track the index. TomPrincipal Solutions ArchitectBetter Software Solutions, Inc. 727-437-2771On Sep 4, 2023, at 10:18 AM, Howie Hoyt @.***> wrote: The call stack looks like:

LBN_Selchange

SpotsList.SetCursor

GetBMSelItemData <<< line 536 uSpots

FCurrentCursorFreq :=

On Mon, Sep 4, 2023 at 9:43 AM Tom Schaefer @.***>

wrote:

What I can’t find is the corresponding SetItemDataPrincipal Solutions

ArchitectBetter Software Solutions, Inc. 727-437-2771On Sep 4, 2023, at

7:52 AM, Howie Hoyt @.***> wrote:

Hi Tom -

it is triggered by Uspots proc TDXSpotslListSetCursor

FCurrentCursorFreq := GetBMSelItemData;

Returns the entry # in the BM.

On Sun, Sep 3, 2023 at 6:29 PM Tom Schaefer @.***>

wrote:

I'm quite confused. I am in the state of How could this possibly work

reliably? When a spot is double-clicked, I see the code get an index by

sending the message, LB_GETITEMDATA. But I cannot find anything related

to

the spot that does the corresponding LB_SETITEMDATA. Typically you would

set an index then store that with the listbox entry. When accessed, you

get

the index (integer) back and use it to index a list (FLIST in this

case).

The other thing that is missing considering this is threaded code is any

sort of locking around the index creation. The pattern should be grab a

lock (TCriticalSection), assign the add the spot to the SpotList object,

add the value to the bandmap list box including the LB_SETITEMDATA

call),

then free the lock. In threaded code (which Telnet is, this is hit or

miss.

I do see some code in uTelnet that uses LB_SETITEMDATA but that is for

the

entry in the telnet window. It might be possible that the whole telnet

record is passed to the spotLIst but that seems odd.

Looking at this code (and it is definitely a big pile of spaghetti), I

would say a completely new object should be rewritten to manage the

spots.

The SpotsList object is the right idea but this code is really rough.

I of course could just not be understanding it fully yet but I am not

following parts of it.

A simpler thing might be to just put a criticalsection around where the

spot is created in the SpotsList and anywhere the SpotsList is modified

(Add, Delete, etc).

Reply to this email directly, view it on GitHub

https://github.com/n4af/TR4W/issues/688#issuecomment-1704421962, or

unsubscribe

<

https://github.com/notifications/unsubscribe-auth/ABVLCUDLI65LRYMSPQL6DBLXYUAC3ANCNFSM6AAAAAA4HYZBPY>

.

You are receiving this because you commented.Message ID:

@.***>

—Reply to this email directly, view it on GitHub, or unsubscribe.You are

receiving this because you authored the thread.Message ID: @.***>

Reply to this email directly, view it on GitHub

https://github.com/n4af/TR4W/issues/688#issuecomment-1705300663, or

unsubscribe

https://github.com/notifications/unsubscribe-auth/ABVLCUHRKVNWW4YTSLCMQNLXYXLHXANCNFSM6AAAAAA4HYZBPY

.

You are receiving this because you commented.Message ID:

@.***>

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: @.***>

n4af commented 9 months ago

i can set bm to update every x filtered entries in the cluster window. setting it to every other entry halves the blinking and updates by grouping in twos. Q: should this be a CTRL-J parm set by the user for 'update very x spots' ? Let me put out a '[every other spot' update]'(https://www.tr4w.net/4.126/tr4w_setup_4_126.beta.exe) and you can see if that works.

n4af commented 9 months ago

TR4W beta2 should greatly reduce the update frequency. BM runs one spot behind the cluster window.

ny4i commented 9 months ago

I recall you mentioned if you slowed it down, the BM item you click on might change to the wrong frequency ()presumably another spot). Is that not an issue with this change?

Tom NY4I

On Sep 20, 2023, at 10:50 AM, Howie Hoyt @.***> wrote:

TR4W beta2 https://www.tr4w.net/4.126/tr4w_setup_4_126.beta2.exe should greatly reduce the update frequency. BM runs one spot behind the cluster window.

— Reply to this email directly, view it on GitHub https://github.com/n4af/TR4W/issues/688#issuecomment-1727886317, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC6TWSWUPUMWR53SUQIK74DX3L7CTANCNFSM6AAAAAA4HYZBPY. You are receiving this because you authored the thread.

n4af commented 9 months ago

it is/remains an issue. I think beta2 reduces the updates substantially tho...

On Wed, Sep 20, 2023 at 10:53 AM Tom Schaefer @.***> wrote:

I recall you mentioned if you slowed it down, the BM item you click on might change to the wrong frequency ()presumably another spot). Is that not an issue with this change?

Tom NY4I

On Sep 20, 2023, at 10:50 AM, Howie Hoyt @.***> wrote:

TR4W beta2 https://www.tr4w.net/4.126/tr4w_setup_4_126.beta2.exe should greatly reduce the update frequency. BM runs one spot behind the cluster window.

— Reply to this email directly, view it on GitHub < https://github.com/n4af/TR4W/issues/688#issuecomment-1727886317>, or unsubscribe < https://github.com/notifications/unsubscribe-auth/AC6TWSWUPUMWR53SUQIK74DX3L7CTANCNFSM6AAAAAA4HYZBPY>.

You are receiving this because you authored the thread.

— Reply to this email directly, view it on GitHub https://github.com/n4af/TR4W/issues/688#issuecomment-1727891390, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABVLCUC5HGWQK3TFDQE65IDX3L7NHANCNFSM6AAAAAA4HYZBPY . You are receiving this because you commented.Message ID: @.***>

df5en commented 9 months ago

4.126.beta2: I can confirm that.

n4af commented 9 months ago

well, we are back to square one. Locking the BM throws it out of sync with the buffer. Stopping the buffer means spots are thrown away.

4.37 introduced 'prevent refresh' -when focus was on the bm. Nice, but it causes the bm<>buffer to be out of sync as does writing buffer entries out in groups of two or three.

The only solution I am aware of is strong filtering of spots. One feature that works well is ctrl-j parameter IE SWITCH - when set to 'true', only spots in trmaster or the initial exchange file will go into the bandmap.

ny4i commented 9 months ago

I do believe using critical sections around the code that  changes where the BM info is stored would help. I just had not made it that far. It has to be done carefully to make sure any exceptions don’t orphan a lock call (try…finally)TomPrincipal Solutions ArchitectBetter Software Solutions, Inc. 727-437-2771813-205-6388 (mobile/text)On Sep 21, 2023, at 9:12 AM, Howie Hoyt @.***> wrote: well, we are back to square one. Locking the BM throws it out of sync with the buffer. Stopping the buffer means spots are thrown away. 4.37 introduced 'prevent refresh' -when focus was on the bm. Nice, but it causes the bm<>buffer to be out of sync as does writing buffer entries out in groups of two or three. The only solution I am aware of is strong filtering of spots. One feature that works well is ctrl-j parameter IE SWITCH - when set to 'true', only spots in trmaster or the initial exchange file will go into the bandmap.

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: @.***>

n4af commented 9 months ago

OK, the CQWWRTTY is underway and a perfect chance to check for rapid updates/blinking.

Log into 'dx.k3lr.com:23' and set filter to 'set dx/filter skimmer': spots flowing from ww skimmers are overwhelming !

Now, I edit file 'cluster_commands.txt' (the tr4w cluster commands file) I add a line so the AR COMMANDS reads:

AR-CLUSTER COMMANDS:

set/dx/filter call=N4AF or (not skimmer and spottercont=NA) or ((skimmer and unique>2) and skimvalid)

and restart TR4W, connect to dx.k3lr.com:23 and then select COMMANDS from the dx cluster window.

One of the lines reads SET FILTER. When I click on it my command (and a lot of other ones previously set) show up. I click on set/dx/filter call=N4AF or (not skimmer and spottercont=NA) or ((skimmer and unique>2) and skimvalid) and suddenly spots roll out in a very organized way every few seconds. Because the continent of NA is so large, the actual command I use is set/dx/filter call=N4AF or (not skimmer and spottercont=NA) or ((skimmer and unique>2) and not skimbusted and (spotterstate=[WV,MD,VA,PA,NC]))

-But point being, that, with proper filtering, the spots rate should not be a problem-

In the case of the k3lr cluster, Tim has excellent documentation at http://www.k3lr.com/w9zrx/AR-Cluster%20Filter%20Commands.pdf

ny4i commented 1 month ago

I added CriticalSections around the code that adds or deletes entries in the bandmap. I also added it to the clear code. If you know a way to duplicate where clicking o9n a spot can have it change to wrong frequency, it would be good to try to see if problem goes away. If so, then the other things discussed here to get bandmap to stop flashing (like delay it), could be implemented. But of course, this has to be validated as not breaking anything. Pull request coming...

ny4i commented 1 month ago

The pull request is in a branch called fix-sub-mode-ssb. I forgot to create a new branch but merging the pull request should work. Again, this is a test release to be evaluated.