Creator refactoring modifies Qt library headers
-
[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 :/
-
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 :)
-
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.
-
And we have a bug for this:
https://bugreports.qt-project.org/browse/QTCREATORBUG-8561 -
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...
-
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 :)
-
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.