Menu

#2270 [Qt] ScintillaEdit has incorrect casts

Bug
closed-rejected
nobody
5
2021-10-25
2021-07-29
No

I am using Scintilla v5.1.1 (Qt 5.15.2, Windows 10) and ran into some odd issues trying to set and get custom properties.

One thing I noticed which could be the cause of this is in the ScintillaEdit class that is generated by WidgetGen.py

It is not properly casting parameters to uptr_t for example it generates:

void ScintillaEdit::setProperty(const char * key, const char * value) {
    send(SCI_SETPROPERTY, (sptr_t)key, (sptr_t)value);
}

However in ScintillaEditBase::send is defined as:

sptr_t ScintillaEditBase::send(unsigned int iMessage, uptr_t wParam, sptr_t lParam) const
{
    return sqt->WndProc(static_cast<Message>(iMessage), wParam, lParam);
}

Notice the 2nd parameter should be sent as uptr_t. Also in ScintillaEdit there are places it calls TextReturner which also doesnt properly use uptr_t

Discussion

  • Neil Hodgson

    Neil Hodgson - 2021-07-29

    Its unlikely this will cause any different behaviour.

    This patch may fix the casts:

    --- C:/u/arc/scintilla/qt/ScintillaEdit/WidgetGen.py    Fri Jul 30 09:11:58 2021
    +++ C:/u/hg/scintilla/qt/ScintillaEdit/WidgetGen.py Fri Jul 30 09:08:47 2021
    @@ -161,7 +161,7 @@
                        if stringResult:
                            returns += "    " + returnStatement + "TextReturner(" + featureDefineName + ", "
                            if "*" in cppAlias(v["Param1Type"]):
    -                           returns += "(uptr_t)"
    +                           returns += "(sptr_t)"
                            if v["Param1Name"]:
                                returns += normalisedName(v["Param1Name"], options)
                            else:
    @@ -170,7 +170,7 @@
                        else:
                            returns += "    " + returnStatement + "send(" + featureDefineName + ", "
                            if "*" in cppAlias(v["Param1Type"]):
    -                           returns += "(uptr_t)"
    +                           returns += "(sptr_t)"
                            if v["Param1Name"]:
                                returns += normalisedName(v["Param1Name"], options)
                            else:
    
     

    Last edit: Neil Hodgson 2021-07-30
    • Neil Hodgson

      Neil Hodgson - 2021-10-25

      A change set [c06ca5] deliberately made these sptr_t to avoid problems with negative numbers.

       

      Related

      Commit: [c06ca5]

  • Justin Dailey

    Justin Dailey - 2021-07-29

    The generated code does 'look' more appropriate.

    Still there is some underlying issue I'm not sure how to exactly reproduce. If I get the problem narrowed down a bit more you want to just continue the discussion on this ticket? Or I can create a new one? Or discuss on the mailing list?

     
    • Neil Hodgson

      Neil Hodgson - 2021-07-30

      The mailing list is better as it includes people that use Qt more.
      https://groups.google.com/g/scintilla-interest

       
      • Justin Dailey

        Justin Dailey - 2021-07-30

        Will do. Thanks!

         
  • Neil Hodgson

    Neil Hodgson - 2021-07-30

    SCI_SETPROPERTY is now implemented by each lexer although there is still some commonality. SCI_SETPROPERTY may not do anything when a property is not supported by a lexer. SCI_GETPROPERTYEXPANDED no longer expands properties.

     
    • Justin Dailey

      Justin Dailey - 2021-07-30

      SCI_SETPROPERTY is now implemented by each lexer although there is still some commonality.
      SCI_SETPROPERTY may not do anything when a property is not supported by a lexer.

      Good to know. I guess there may not be an issue after all. I think there was a misunderstanding on my part as to how properties could be used.

      With Scintilla v4.x I was able to set dynamic properties (I thought they were part of the document, but sounds like that's wrong) which means I did something like:

      ScintillaEdit *edit = new ScintillaEdit(this);
      edit->ScintillaEdit::setProperty("nn.meta.property", "foobar");
      qInfo() << "property: " << edit->ScintillaEdit::property("nn.meta.property");
      

      With the move to Scintilla v5 and Lexilla I'll find alternative methods since I didn't realize I was leveraging non-desirable behavior :) Thanks for the info.

       
      • Neil Hodgson

        Neil Hodgson - 2021-07-30

        The problem was where to implement the property behaviour now that lexers are independent from Scintilla: each lexer now has to store the properties that control it. It would make it more difficult to implement lexers if they each had to support arbitrary property names and provide the property expansion feature as well.

        Having Scintilla store all properties just so these features would work runs into other issues: which module would then be the authoritative source of data? Applications can directly call a lexer's PropertySet method. The lifetime of values is also uncertain - whether they should outlive changing lexers on a Scintilla instance and whether they should revert when switching back to a previous lexer. There were already similar issues back in 4.x but Lexilla made them more acute.

         
  • Neil Hodgson

    Neil Hodgson - 2021-10-25
    • labels: --> scintilla, qt
    • status: open --> closed-rejected
     
  • Neil Hodgson

    Neil Hodgson - 2021-10-25

    Closing this as the underlying problem is 'by design'. The suggested changes do not appear to be worth applying as they revert an earlier fix.

     

Log in to post a comment.