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
qwidgetBasically, 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. -
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.
-
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 :/
-
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 :)