Creator refactoring modifies Qt library headers



  • I just noticed what I think is a serious potential flaw in Creator - using refactor to rename a mouseReleaseEvent to a mousePressEvent for my project files files actually went ahead and applied the change to a bunch of stock Qt headers that weren't actually opened:

    qabstractscrollarea
    qabstractspinbox
    qlabel
    qgroupbox
    qabstractitemview
    qheaderview
    qabstractbutton
    qtextedit
    qplaintextedit
    qtabbar
    qslider
    qcombobox
    qlineedit
    qwidget

    Basically, it went and refactored the headers for absolutely every component used in another opened, but non active project. I reiterate - the files were not open in Creator, the refactoring feature probably leaked through moc generated files.

    And the worst part is you refactor, press rebuild and every one of those files is automatically saved and is rendered faulty because of the redefinition.

    I think this is very disastrous default behavior that needs to be addressed.

    I really think Creator refactoring should look neither in files that are not opened, nor in files that do not belong to the active project in the first place.



  • Isn't that impossible in Linux? Since every header is installed in /usr/include wich belongs to root, there will be no problem with that.
    I think there should be such a restriction for Windows too. Maybe installers should run with administrative permissions and set installed files as read-only. I don't know if it's possible in windows or not.
    Though refactoring mechanism seems sane to me. It should dive deep into headers and apply changes recursively. Making an exception, made it's behavior somehow irregular. In my opinion, programmer should take care about refactoring. And of cource should take care not using same names with Qt's internals, or rename something not defined by programmer.


  • Moderators

    That doesn't really seem reasonable, IMHO, too. Have you filed a bug report?



  • [quote author="soroush" date="1358191626"]And of cource should take care not using same names with Qt's internals, or rename something not defined by programmer. [/quote]

    When you are subclassing, extending and overloading existing Qt classes you have no option but to use the same names...

    Also, diving into files that are neither opened nor part of the active project is definitely something that refactoring SHOULDN'T DO!

    bq. Have you filed a bug report?

    I will when I have the time.



  • [quote author="utcenter" date="1358192109"]
    When you are subclassing, extending and overloading existing Qt classes you have no option but to use the same names...
    [/quote]

    Hmm... I don't think so. Qt Creator will not be confused about same names in base classes / derived classes with same names in other classes. If so, I would call it a bug. If not, it's ok to me.

    Update: Well, seems that it does. This may be a good idea in some situations, but personally I didn't like it. This is probabely discuessed by developers of Qt Creator, Though I would call this a bug not a feature :{

    Update 2: Ahh... My bad. It's OK.


  • Moderators

    bq. When you are subclassing, extending and overloading existing Qt classes you have no option but to use the same names…

    Indeed. I can see, though, where this might be a use case where the refactor tool probably wasn't very thoroughly tested (that's just speculation, of course) as I imagine that refactoring a mousePressEvent into a mouseReleaseEvent might not be the most commonly thought of use case. It sounds like, though, that the tool did what was intended (i.e., went back through the source tree and mechanically made the changes.) I would hope there would be better safeguards against changing the base libraries, of course. I'd fall into the camp of calling it a bug, as well.

    As for only changing open files, I can see where one could make a case either for or against that. In a perfect world, it would probably best be something that could be configurable to the user's preferences -- like how the ToDo plugin allows you to pick open files vs. the project tree.



  • [quote author="soroush" date="1358192342"]
    [quote author="utcenter" date="1358192109"]
    When you are subclassing, extending and overloading existing Qt classes you have no option but to use the same names...
    [/quote]

    Hmm... I don't think so. Qt Creator will not be confused about same names in base classes and derived classes. If so, I would call it a bug. [/quote]

    When I did the refactoring, I assumed it will only refactor the files in the currently active project. That is after all the logical thing to do.

    Instead it went to the inactive project, to a moc generated file that wasn't even opened, and from there to every header that was included into more files that were not opened.

    This renamed every mouseReleaseEvent to a mousePressEvent in 14 headers.

    Also, duplicating identifies is pretty much unavoidable, not only when you overload already existing methods, where you HAVE TO use the same signature, but also in many accessor methods like setValue, setText, doWork, isEmpty which are common names in programming conventions and are being used all the time for the sake of consistency.

    mlong- I just think no one ever did that kind of consideration, even though it is pretty obvious, this would represent a problem not only when overloading but in many other cases of commonly used identifies I mentioned above.

    I really don't think refactoring should leak outside of project files - you normally don't want to refactor your entire library, do you? If the library is the opened active project - then yes, but otherwise, when you are only using the library - BAD IDEA... :)

    And yes, it sounds like a bug because IT DOESN'T LEAK through the files, included in the active project, it leaked through the generated UI files of the inactive project.

    In other words, refactoring does not go through your library includes as you both seem to claim is reasonable - it did through a place the implementers of refactoring didn't consider apparently...



  • Consider following code:
    @
    class A
    {
    public:
    virtual void foo();
    };

    class X
    {
    public:
    virtual void foo();
    };

    class Y: public X
    {
    public:
    void foo();
    };
    //Somewhere in code:
    Y a;
    a.foo(); // Ctrl+Shift+R here and choose bar instead of foo
    @

    After refactoring it should be something like:

    @
    class A
    {
    public:
    virtual void foo();
    };
    class X
    {
    public:
    virtual void bar();
    };
    class Y: public X
    {
    public:
    void bar();
    };
    //Somewhere in code:
    Y a;
    a.bar(); // Ctrl+Shift+R here
    @

    This looks fine. After all, the Creator asks you to choose which names should be refactored:
    !http://s2.picofile.com/file/7620641177/sc28.png(ask)!

    Seems problematic a little. Not a disastrous behavior :/


  • Moderators

    bq. I really don’t think refactoring should leak outside of project files – you normally don’t want to refactor your entire library, do you?

    I agree. I would think it should only take into consideration files which are in your current project. (Whether it is limited to only files open for editing is another conversation altogether.)

    bq. I just think no one ever did that kind of consideration, even though it is pretty obvious, this would represent a problem not only when overloading but in many other cases of commonly used identifies I mentioned above.

    Indeed. That was exactly the point I was trying to make :)

    [Edited to remove a suggestion that the refactor tool should preview the changes first -- which I forgot that it does]



  • ^^^ I didn't mean it should only affect files that are opened and ignore project files that are not, I just specified that the files that got modified were in no way open, be that as a part of a project or as standalone files, as using the "Follow symbol under cursor" feature does.

    soroush - you are completely missing the point :)


  • Moderators

    bq. ^^^ I didn't mean it should only affect files that are opened and ignore project files that are not, I just specified that the files that got modified were in no way open, be that as a part of a project or as standalone files, as using the "Follow symbol under cursor" feature does.

    We're in complete agreement, then, I think.





  • OK, I can't understand you utcenter O.o

    • I wouldn't divide the universe into physical parts like projects, libraries, applications, etc... I would rather prefer watching the world from a compiler's vision. There is not a project in my definition. There are symbols, scopes, files and binaries. So I have no problem with refactoring things that they are present in current visible scope (recursively).
    • A symbol should be refactored by its semantic I believe. Which is already done by the tool in a perfect manner. When I choose a symbol to be refactored, I would not except the creator to rename every occurrence of that word. It's not important in which file, open or closed, in my project or somewhere else... I like it to change declaration of the symbol and it's all visible occurrences. Visible means that all files (either header or source) included, or to be processed by the compiler.

    Putting these parts together, I think I'm happy with creator's refactoring mechanism.

    Maybe I'm missing the point as you said...



  • So, if your dog is called Sparky, and your dog gets lost and found by someone else, who renames the dog to Russel, all dogs named Sparky in the universe should also be renamed Russel? Shouldn't only that particular dog be renamed, instead of all existing Sparky dogs?

    You still fail to understand that this is a BUG, it is not part of the intended refactoring mechanism. Refactoring doesn't go to the headers you include and doesn't do changes there. It only leaks through the files, that were generated from the Designer UI form.

    In most cases refactoring works exactly the way it should (and not the way you claim it should) but in this particular case, for some reason that was not considered when refactoring was implemented, it works in an unintended way that has the potential of creating unintended changes in unintended places.



  • That's exactly the point!

    Only that particular dog is renamed. Please consider that a virtual method belongs to both base and derived classes.

    Try this code:

    @
    class X
    {
    public:
    virtual void bar();
    private:
    void P();
    };

    class Y: public X
    {
    public:
    void bar();
    private:
    void P();
    };

    int main()
    {
    Y a;
    a.bar();
    a.P(); // Refactor here and see the result
    return 0;
    }
    @

    As you can see, a private method is refactored as it should be. (Only in class Y), No matter that base class have a method with same name. It's unchanged. You are trying to rename not refactor.

    It still seems OK to me! Let's wait and see what devlopers of project will do with your bug-report.



  • This code doesn't meet the criteria needed for the bug to occur, did you actually read any of what I posted?

    You need to have another inactive project with a UI form and attempt to refactor an identifier that exists in the headers of the widgets, used in the form, and the form needs to be processed by the moc and its source code generated - all this you need for the bug to occur.

    Here is it, I click to refactor MyLineEdit::mousePressEvent() - and see how much other classes get modified:

    !http://i48.tinypic.com/1zfh9ol.png( )!

    I also notice it wrongly picks it as QWidget::mousePressEvent



  • [quote author="utcenter" date="1358196332"]This code doesn't meet the criteria needed for the bug to occur, did you actually read any of what I posted?
    [/quote]
    Yes, I already said that I don'd divide the code universe into physical parts! I can see only logical parts.
    [quote author="utcenter" date="1358196332"]
    You need to have another inactive project with a UI form and attempt to refactor an identifier that exists in the headers of the widgets, used in the form, and the form needs to be processed by the moc and its source code generated - all this you need for the bug to occur.[/quote]
    Still same.



  • I encourage you to go back to the previous page and take a look at the image I posted, and if you still claim this is the correct behavior of refactoring my bet is you really don't have anything better to do than being a jackass...


  • Moderators

    Hey... hey... I think the point has been made, and a bug report has been filed. This is descending into something out of scope. Soroush has said he's content with seeing what the developers do with the bug report.

    This does not need to turn into nastiness and name-calling over the specifics of some sub-detail.



  • Yeah yeah, you are right, me and my temper :P That is the downside of the internet, everyone dares to be aggravating when it is not face to face... I actually don't have such problems in real life, I guess the formidable temper just doesn't make the same impression without the formidable appearance :)


  • Moderators

    Ok. So we're cool. :)

    I'm curious to see where this goes, myself. Voted & Watching the bug report, here.



  • Cool like a fool in a swimming pool :P



  • [quote author="utcenter" date="1358197313"]I encourage you to go back to the previous page and take a look at the image I posted, and if you still claim this is the correct behavior of refactoring my bet is you really don't have anything better to do than being a jackass...[/quote]

    I think I clearly explained my thoughts about refactoring and its meaning. I'm gonna wait for the bug-report.



  • You basically said that refactoring only changes the identifier for a particular class.

    In my case it not only fails to detect the correct class but also changes the identifiers for a bunch of other classes that are not even included in the project, I am really amazed that you still fail to see that, and the conflict it is with what you say is right.

    Anyway, this will be the last time I address you for a while, cause I feel like paying attention to you only motivates you, still ... feel free to continue blabbering...



  • Okay, this is the last time I'm trying to explain what I expect from a refactoring mechanism.

    A virtual method is a shared symbol between derived and base classes. Refactoring such a method, should rename all instancecs of all subclasses.

    You do include all of those headers in your project. Do you have Qt += gui in your pro file? See compiler command line. All files in visible scope for compiler. All classes you can use in code, with your current include directives, are included.

    You are renaming a virtual method (mousePressEvent)

    Note: Your aggressiveness is not constructive nor encouraging someone to discusse with you. It's not a honor to ridicule somebody even if she/he is so wrong.



  • OK, breaking my own word, but I actually agree that a virtual method is a shared one. But still that doesn't mean refactoring should blindly affect library headers, it should not mess with the API a project is using, it should only be allowed if the API itself is the active project. t should be accounted whether a hierarchy begins at project level or library level and the library API should be immune to refactoring, because that is not something anyone working in a context of using the library would want.


  • Moderators

    IMO, the problem arises because the tool is fairly mechanical and just blindly traverses the code base and, as you point out, in fact tries to do what it's supposed to do by refactoring everything in scope.

    However, I would hope that we can all agree that trying to refactor the Qt headers, themselves, is a behavior which is not generally wanted under any practical circumstances, and the expected behavior of a Qt-centric IDE would be for it to have some semblance of knowledge of where the line of Qt library stuff ends and user stuff begins. As such, I think most people would view this behavior as a bug, regardless of the academic validity of the stance that -- true -- the code that it is changing are indeed within the encompassing scope of the virtual inheritance tree and so on.

    But it's silly for the refactoring tool to try to refactor the Qt headers, themselves, though. I think that's what this comes down to.

    To me it seems reasonable that the reach of the refactoring tool should not automatically step outside the bounds of files contained in the current .pro itself. At least not without a warning or something.

    bq. Your aggressiveness is not constructive...

    That was discussed above.



  • @soroush - I have a question for you:

    If you are correct, then why isn't refactoring modifying all QWidget derived classes, which are quite a few more than the ones I get modified, as you claim it should? After all, I am refactoring a virtual method, and thus all of its instances should be refactored?? Instead it only modifies the classes, used in the inactive open project. What is your metaphysical theory on that kind of behavior? ;)



  • moc-generated files contains only #includes of widgets used in project. So only visoble classes will be refactored.



  • But if "out of project scope" is the intended behavior of refactoring, doing so will break all the non-visible classes since the base virtual method will effectively disappear?

    Not to mention that if was indeed intended for refactoring to be able to affect classes at the library level, it should not only affect all headers, regardless if they are visible or not, but also the cpp files, because otherwise everything will break.

    Or maybe such a behavior for refactoring was not intended in the first place, or if intended, was not implemented thoroughly enough?

    Fact is refactoring ... needs some more refactoring if it is intended to be used outside of a project scope on global level. Otherwise it obviously breaks the code.

    In short - out of project scope changes cannot be made in the blind as refactoring currently operates.


  • Moderators

    I personally maintain that "out of project scope" is not (nor was it ever) the intended behavior, but rather is the bug that needs to be addressed. Developers want to refactor the code in their project, not the entire development API.

    A bug report has already been filed... it's done.

    Why are we even still talking about this? Just let it go... really... just let it go...

    Please call off this stupid pissing match, or take it to email, or something...

    [I think I need to call it a night. I'm unsubscribing to updates. Where's the Tylenol?]



  • @mlong - what's the sweat, I think we are back at the level of civilized discussion, I got his point and now we are iterating over it.

    If this is indeed a bug, it is a product of the "lack of intelligence" in refactoring, because right now it is literally blind. It should be improved upon by either:

    • making it intelligent enough to keep it to the active project. Note I say "active project" rather than "open projects"

    or...

    • making it intelligent enough to know better when it steps out of active project scope. Which is arguably a good idea to begin with, since the little benefit this approach has is completely overshadowed by the disruption to APIs it can cause.

    In short, I'd rather maintain my position that the API should be immune to refactoring when it is being used, and should only be ... refactorable?!? if it is the active project.

    This can be achieved by constraining refactoring from tracking hierarchies to project level and no deeper. Which while simple, may not be easy to do, since I suspect refactoring borrows its "intellisense" from a source that is shared between other functionality, e.g. refactoring logic should be reimplemented.

    This way it would do what I expected it to do when I found this "bug" - change the method name for MyLineEdit from mousePressEvent to mouseReleaseEvent, so it corresponds to QWidget::mouseReleaseEvent, instead of changing every visible to Creator QWidget derived class' press even to a release event.


  • Moderators

    bq. what’s the sweat...

    Yeah, this is what I get for being online when I'm sick. :-) Patience wears thin sometime. Apologies. I feel better after a good night's sleep...

    I agree with the rest of the sentiment about improvements which ought to be made. That's the beauty of the living nature of a product like this, in that we can take something and improve upon it.

    Granted, the refactor tool is still pretty helpful as it stands, so long as we are aware of limitations inherent in the current implementation. I have nothing but respect for those who worked hard to create this tool in the first place. I use it all the time and haven't had any problems (granted, I'm usually only renaming my own methods and variables, not inherited methods.

    And I do think that discussion of the shortcomings, preferred operation, preferences, and so on are nothing but beneficial (even when unfortunately tinged with pointy and prickly language :-P ) and can help guide the way these things are implemented and improved in the future.

    +1 on the position that the API should be immune.
    +1 on limiting to the active project scope

    Anyway, just my (more levelheaded) thoughts this morning... carry on.



  • Also, another thought I have on this - it would be nice to be able to do context aware refactoring. What I mean by that is:

    @class BaseClass ...

    class MyClass : public BaseClass ...@

    If I click refactor on the first BaseClass identifier it is logical to assume I want to refactor the name of this class in the entire (project) hierarchy.

    BUT if I click refactor on BaseClass in the context of a class I am inheriting from, it would be nice to refactor it in that context and change the class name of the class that MyClass inherits from... in the declaration, definition, constructor and potential calls to base class methods in the body of MyClass. Makes sense, doesn't it?


  • Moderators

    Please file bug reports about the features you want! Stuff reported here will be lost.



  • I've been considering a feature suggestion. The current refactoring mechanism is literally blind, dumb and deaf, and that is OK in most of the cases, not to mention many experience developers are used to such behavior. So I don't see a reason for this to change.

    But I also don't see a reason not to introduce an additional "smart refactor" tool that distinguishes between API/library and project scope, respects the context of the refactored item and offers the option to keep it to current class or reach all the way down into virtual hierarchies in the project or if needed all the way into the API.



  • I just got bitten by this one in a big way...had to reinstall in order to fix all the broken headers...


Log in to reply
 

Looks like your connection to Qt Forum was lost, please wait while we try to reconnect.