2009/7/7 Thorbjørn Lindeijer <thorbjorn.lindei...@nokia.com> > > Well, you're not breaking any fundamental design principles, but I do > have some comments. :) >
Excellent. And good comments too, thank you. :) > > It's not really nice that you have to do the "if (!isModified() && > Core::EditorManager::instance()->alwaysReload())" check in every > editor's modified() method. Personally, I think you could put this check > in FileManager::checkForReload(), and use a temporary IFile::ReloadAll > behaviour (as done in EditorManager::revertToSaved()), to make each > unmodified editor reload without asking questions. > Righto. That is indeed a better spot. Had I found FileManager::checkForReload() on my own, I'd have put it in there. :) The updated patch behavior works by setting the initial value to what was specified in the config dialog. If the user choses a "for all" dialog option for some file, that will apply to all modifications in that batch and override the config dialog setting. I think that is what you want, but I'm a bit unsure. > When the above works out, I think the option should probably move into > FileManager instead of EditorManager, but it's not that important. I see > the FileManager doesn't have its own settings yet. > I've left it in EditorManager for the moment. > > An accessor like EditorManager::alwaysReload() should be const. > Yes, of course. (somewhat of a d'oh, how did I forget that? ) > > By not using a bool but an enum, you could make even more people happy. > I've read requests for never asking for reload as well, so what about > having a combo box showing three options? > > Reload externally modified files: > * Always ask > * Ask only with unsaved changes > * Never reload > > I'm not entirely sure whether these options are immediately clear > though. And at the moment I don't think we should have a "Without > asking" option, since I consider that quite dangerous. I'm not sure what > the use-case is of editing a file in Qt Creator but then wanting those > changes to be thrown away without asking. > Indeed. An enum is a better idea. I completely agree that a "Reload modified without asking" is too dangerous to be useful. I'm doing some implicit conversations between ints and the IFile::ReloadBehavior enum in this patch, for example when storing it in the config system. As far as I know, an enum is an integral type in C++98 and 03 and the first element will have a value of zero and the following values will have succeedly increasing values which are exactly the guarantees I depend on. Nonetheless, it might feel a bit dirty (it creates some gotchas if you want to modify IFile::ReloadBehavior). Let me know if the house style prefers explicit handling of the values. > That's all. Looking forward to a revised patch. :-) Here it is: http://qt.gitorious.org/~abelsson/qt-creator/abelssons-clone/commit/21abdba6290bc671fa965a092b1f6c4b55d6d997 -henrik
_______________________________________________ Qt-creator mailing list Qt-creator@trolltech.com http://lists.trolltech.com/mailman/listinfo/qt-creator
