Opened 7 days ago

Closed 3 days ago

#17949 closed enhancement (fixed)

Add Scintilla FineTicker methods to wxSTC

Reported by: NewPagodi Owned by: Artur Wieczorek <artwik@…>
Priority: normal Milestone:
Component: wxStyledText Version: dev-latest
Keywords: timer Cc: fuscated@…, m0081@…
Blocked By: Blocking:
Patch: yes

Description

Starting with version 3.5, Scintilla implemented a newer method for handling timers and used this method in its windows, gtk+, cocoa, and qt ports. This patch attempts to bring the timer handling for wxSTC in line with those other ports.

Currently, wxSTC overrides the Editor::SetTicking(bool) method which creates/destroys timers as needed throughout the lifetime of the control.

In contrast the newer way of handling timers creates 4 timers in the controls constructor (and deletes them in the destructor) - one timer for each of the 4 reasons that a timer might be needed. The control then uses the virtual methods FineTickerRunning, FineTickerStart, and FineTickerCancel to manage these timers.

Also the older Editor::Tick method has been superseded by the Editor::TickFor(TickReason) method where TickReason is an enumerated type defined for the Editor class.

To give the time timers access to this enumerated type and the TickFor method, I modified the wxSTCTimer class and made it a friend class of ScintillaWX. I think think this is the simplest and most natural way of handling the timer reasons, but I'll understand if this is considered undesirable.

I followed the advice given by the main Scintilla developer here https://groups.google.com/forum/#!topic/scintilla-interest/10VqrkKR2MI to try to keep this as future proof as it can be. I made one change to the advice given. The comments in the link advised using a std::array to hold the set of timers. Since the wxWidgets coding guidelines say not to use standard containers, I used a wxVector instead.

Attachments (3)

stctimers.patch download (5.1 KB) - added by NewPagodi 7 days ago.
add FineTicker* methods to wxSTC
stctimers_wofor.patch download (5.6 KB) - added by NewPagodi 5 days ago.
Initialize timers one by one
stctimers_whash_donotuse.patch download (5.3 KB) - added by NewPagodi 4 days ago.
Using a wxHashMap for the timers - crashes as is - do not use

Download all attachments as: .zip

Change History (15)

comment:1 follow-up: Changed 7 days ago by awi

I would have a remark regarding association between wxSTCTimer instance and a TickReason. Reason identifier is stored in the timer instance, but in the functions like FineTickerRunning(), FineTickerStart(), FineTickerCancel(), timer for given reason is identified by the index in the array of timers, not by internally stored identifier. This introduces some redundancy and also coupling between reason id's and the representation of the array of timers. I think that decoupling id's from the array would be a future proof solution.

comment:2 in reply to: ↑ 1 Changed 7 days ago by NewPagodi

  • Cc m0081@… added

Replying to awi:

I agree that's a little redundant, but the problem is that the timer needs to call Editor::TickFor(TickReason) in its notify method. When that comes up, the timer doesn't know its position in the vector, so the only way I can figure out how to do that is to store it it the timer itself. Am I missing an easy solution here?


The problem I'm most concerned with is that we need to set up a timer for each member of the TickReason enum, but since c++ doesn't have a way to iterate over enums, the only thing that can be used is a for loop starting at the first member and ending at the last member.

So if new TickReasons are ever added at the end of that enum in the Scintilla code, then that for loop in the STC code won't set up a timer for those reasons. In the thread from the Scintilla list I mentioned above, it's said that there probably won't be any new reasons added; but if something unforeseen comes up and is added, it will be added before the current last member. As long as that's the case, the vector of timers should continue to work as intended.


Actually I've changed the patch to add a few more checks to make sure I never try to use a null pointer. That shouldn't be possible, but I thought it best to be double sure. If I'm being overly cautious, I can take them out.

Changed 7 days ago by NewPagodi

add FineTicker* methods to wxSTC

comment:3 Changed 7 days ago by obfuscated

  • Cc fuscated@… added

comment:4 follow-up: Changed 6 days ago by awi

Your concern regrading representation of TickReason as enum is entirely justified. Your implementation bases on the assumption that reason codes occupy continuous block of indices within the range [tickCaret..tickDwell], but this is not guaranteed and can be the subject of change without any notice. I think it would be good to protect your implementation against this factor.
Maybe in the functions like FineTickerRunning() it would be good to rely only on the reason id stored in the object instead of its index in the table?
In ctor, perhaps timers could be created one by one for every known TickReason instead of being created in the loop?

comment:5 in reply to: ↑ 4 Changed 6 days ago by NewPagodi

  • Cc m0081@… removed

Replying to awi:

I've added a second version of the patch that does what I think you're asking for. It will be slightly less efficient and will miss timers if the TickReason enum is ever changed (but it's unlikely that it will ever be changed).

For triple safety should we add an assert if we ever encounter an unknown reason so that it can be caught and fixed? Something like the following:

void ScintillaWX::FineTickerStart(TickReason reason, int millis,
                                  int WXUNUSED(tolerance)) {
    bool found(false);
    for ( wxVector<wxSTCTimer*>::iterator i=timers.begin();
          i!=timers.end(); ++i ) {
        if ( reason==(*i)->GetReason() ) {
            (*i)->Start(millis);
            found=true;
            break;
        }
    }
    wxASSERT_MSG(found, "At least one TickReason is missing a timer.");
}

comment:6 follow-up: Changed 5 days ago by awi

As for me, it looks more Scintilla-proof now! :)
If performance of iterating through the collection of timers worries you, maybe you could take a look at wxHashMap (if I remember the name correctly) as a container for timers (instead of wxVector)?

Changed 5 days ago by NewPagodi

Initialize timers one by one

comment:7 in reply to: ↑ 6 Changed 5 days ago by NewPagodi

  • Cc m0081@… added

Replying to awi:

As for me, it looks more Scintilla-proof now! :)
If performance of iterating through the collection of timers worries you, maybe you could take a look at wxHashMap (if I remember the name correctly) as a container for timers (instead of wxVector)?

I tried that for investigative purposes, but I couldn't get it to work. I can make it work with a standard map, but when I try to use a wxHashMap an application using wxSTC crashes in the destructor with an unhelpful warning about heap corruption.

I did make one additional change to move the destruction of timers until after the Finalise() method (in case that method ever tries to do anything with the timers) and then clear the vector (so that if any of the base class destructors try to call the timer methods, the calls will just fail gracefully).

comment:8 follow-up: Changed 4 days ago by awi

Have you tried wxHashMap with declaration like this?

WX_DECLARE_HASH_MAP(TickReason, wxSTCTimer*, wxIntegerHash, wxIntegerEqual, TimersCollection);

TimersCollection timers;

Changed 4 days ago by NewPagodi

Using a wxHashMap for the timers - crashes as is - do not use

comment:9 in reply to: ↑ 8 Changed 4 days ago by NewPagodi

  • Cc m0081@… removed

Replying to awi:

Have you tried wxHashMap with declaration like this?

WX_DECLARE_HASH_MAP(TickReason, wxSTCTimer*, wxIntegerHash, wxIntegerEqual, TimersCollection);

TimersCollection timers;

I've attached another version of the patch that shows what I've tried. It's basically the same declaration, but visual c doesn't like having TickReason as the key, so I had to fallback to using int instead.

If you just change the line

WX_DECLARE_HASH_MAP(int, wxSTCTimer*, wxIntegerHash, wxIntegerEqual, TimersHash);

to

typedef std::map<int,wxSTCTimer*> TimersHash;

everything works. But with the wxHashMap it always crashes in the destructor. Am I just using wxHashMap wrong?

If it helps, here's the last of the debug log for the stc sample before the crash.

0:000> t
wxmsw311ud_stc_vc_custom!free_dbg:
00007ffe`a2ef011a ff2578430f00    jmp     qword ptr [wxmsw311ud_stc_vc_custom!_imp__free_dbg (00007ffe`a2fe4498)] ds:00007ffe`a2fe4498={ucrtbased!_free_dbg (00007ffe`a709ea70)}
0:000> t
ucrtbased!_free_dbg:
00007ffe`a709ea70 89542410        mov     dword ptr [rsp+10h],edx ss:00000035`ffefec88=1fb8bfa0
0:000> gu
wxmsw311ud_stc_vc_custom!operator delete+0x18:
00007ffe`a2eee308 4883c428        add     rsp,28h
0:000> t
wxmsw311ud_stc_vc_custom!operator delete+0x1c:
00007ffe`a2eee30c c3              ret
0:000> t
wxmsw311ud_stc_vc_custom!operator delete+0x18:
00007ffe`a2eed5a8 4883c428        add     rsp,28h
0:000> t
wxmsw311ud_stc_vc_custom!operator delete+0x1c:
00007ffe`a2eed5ac c3              ret
0:000> t
wxmsw311ud_stc_vc_custom!wxSTCTimer::`scalar deleting destructor'+0x31:
00007ffe`a2ca20b1 488b442430      mov     rax,qword ptr [rsp+30h] ss:00000035`ffefed10=000001ff1fb8bea0
0:000> t
wxmsw311ud_stc_vc_custom!wxSTCTimer::`scalar deleting destructor'+0x36:
00007ffe`a2ca20b6 4883c428        add     rsp,28h
0:000> t
wxmsw311ud_stc_vc_custom!wxSTCTimer::`scalar deleting destructor'+0x3a:
00007ffe`a2ca20ba c3              ret
0:000> t
wxmsw311ud_stc_vc_custom!ScintillaWX::~ScintillaWX+0xce:
00007ffe`a2cb43de 90              nop
0:000> t
wxmsw311ud_stc_vc_custom!ScintillaWX::~ScintillaWX+0xdf:
00007ffe`a2cb43ef e976ffffff      jmp     wxmsw311ud_stc_vc_custom!ScintillaWX::~ScintillaWX+0x5a (00007ffe`a2cb436a)
0:000> t
wxmsw311ud_stc_vc_custom!ScintillaWX::~ScintillaWX+0x5a:
00007ffe`a2cb436a 488d4c2440      lea     rcx,[rsp+40h]
0:000> t
wxmsw311ud_stc_vc_custom!ILT+43415(??EiteratorTimersHash_wxImplementation_HashTableQEAAAEAV01XZ):
00007ffe`a2c7b99c e9bfe40300      jmp     wxmsw311ud_stc_vc_custom!TimersHash_wxImplementation_HashTable::iterator::operator++ (00007ffe`a2cb9e60)
0:000> t
wxmsw311ud_stc_vc_custom!TimersHash_wxImplementation_HashTable::iterator::operator++:
00007ffe`a2cb9e60 48894c2408      mov     qword ptr [rsp+8],rcx ss:00000035`ffefed10=000001ff1fb8bea0
0:000> t
wxmsw311ud_stc_vc_custom!ILT+43400(?PlusPlusIteratorTimersHash_wxImplementation_HashTableIEAAXXZ):
00007ffe`a2c7b98d e94ef10300      jmp     wxmsw311ud_stc_vc_custom!TimersHash_wxImplementation_HashTable::Iterator::PlusPlus (00007ffe`a2cbaae0)
0:000> gu
wxmsw311ud_stc_vc_custom!TimersHash_wxImplementation_HashTable::iterator::operator+++0x13:
00007ffe`a2cb9e73 488b442430      mov     rax,qword ptr [rsp+30h] ss:00000035`ffefed10=00000035ffefed50
0:000> gu
wxmsw311ud_stc_vc_custom!ScintillaWX::~ScintillaWX+0x64:
00007ffe`a2cb4374 90              nop
0:000> gu
wxmsw311ud_stc_vc_custom!ScintillaWX::`scalar deleting destructor'+0x17:
00007ffe`a2cb9ee7 8b442438        mov     eax,dword ptr [rsp+38h] ss:00000035`ffefedb8=00000001
0:000> gu
Debug Error!

Program: ...sionControl\git\wxWidgets\samples\stc\vc_mswuddll\stctest.exe

HEAP CORRUPTION DETECTED: after Normal block (#11192) at 0x000001FF1FB400B0.
CRT detected that the application wrote to memory after end of heap buffer.

comment:10 follow-up: Changed 4 days ago by awi

I have no problems with your implementation with wxHashMap using VS 2010 and 2015 (stc sample). Which complier do you use? Could you try a build with static linking?

comment:11 in reply to: ↑ 10 Changed 4 days ago by NewPagodi

  • Cc m0081@… added

Replying to awi:

I have no problems with your implementation with wxHashMap using VS 2010 and 2015 (stc sample). Which complier do you use? Could you try a build with static linking?

I was using vc2017. I just tried using 2015 both static and dynamic and didn't have any problems. Then I retried with 2017 both static and dynamic and also didn't have any problems - so I guess I must have missed deleting something before the last time I rebuild the library. Sorry about that.

In short, it would seem the version with wxHashMap does work if that's the version you want to use.

comment:12 Changed 3 days ago by Artur Wieczorek <artwik@…>

  • Owner set to Artur Wieczorek <artwik@…>
  • Resolution set to fixed
  • Status changed from new to closed

In 33ad8063d/git-wxWidgets:

Add Scintilla FineTicker methods to wxSTC

Starting with version 3.5, Scintilla implemented a newer method for
handling timers and used this method in its Windows, GTK+, Cocoa, and Qt
ports. This patch attempts to bring the timer handling for wxSTC in line
with those other ports.

Closes #17949.

Note: See TracTickets for help on using tickets.