Reload project view, and make sure that BazelImportSettings respect p… #7088
Conversation
dkashyn-sfdc
requested review from
mai93,
tpasternak and
agluszak
as code owners
github-actions
bot
added
product: CLion
|
Last 2 runs after However, there are some gaps so "toggle" mechanism is essential. |
| * so we cannot reload project view from for every of such call. | ||
| * This is why we have this special case to make sure that Sync respects project view selection if there is any. | ||
| */ | ||
| public static ProjectType getProjectTypeBeforeSync(@Nonnull Project project) { |
There was a problem hiding this comment.
I've been also looking for a way to jump between qs/as on the fly, thanks for submitting this PR! Can we add some javadoc here to reflect somthing like returns the project type with project view file lookup? and maybe make the name more matching the action it does rather than a point it code where it should be called?
There was a problem hiding this comment.
There are 15 places where Blaze.getProjectType*(project) == ProjectType.QUERY_SYNC check is performed. So I wanted to have a special case for sync calls only. This is why name reflects this.
I don't really know how frequent the rest of places are hitting this method and saw it called from action apperance update, which cannot afford to reload file for sure.
So instead of having "get type with refresh" I'd rather keep "get type for sync and make sure the proper sync mode is used". Also we are updating BazelImportSettings here and it is ok to update those on sync but for the rest of ops I'd like to avoid this.
There was a problem hiding this comment.
I can rename it to getSelectedProjectTypeBeforeSync or so. The fact that BazelImportSetings are updated is important, yet subtle here. Before, this change it was set once during project creation, and not touched afterwards, for whatever reason.
Overall, Blaze.getProjectType*(project) == ProjectType.QUERY_SYNC is someway out of control... It is peppered over the code quite randomly, and I'm a little bit worried about it causing trouble in the future even for cases beyond sync.
There was a problem hiding this comment.
getRefreshedProjectType or getNotCachedProjectType is another option... But then it will be a dilemma where to call those :)
| @@ -61,6 +54,27 @@ public BlazeProjectData loadProject(BlazeImportSettings importSettings) { | |||
|
|
|||
| @Override | |||
| public void saveProject(BlazeImportSettings importSettings, BlazeProjectData projectData) { | |||
| if (projectType != Blaze.getProjectType(project)) { | |||
| initBlazeProjectDataDelegate(project); | |||
There was a problem hiding this comment.
Probably we need to call save here for the old model... But this is a thing only for Aspect one.
There was a problem hiding this comment.
With 50% of non-test impls not using saveProject this interface definitely needs some cleanup...
// TODO(mathewi) Tidy up the interface to remove this unnecessary stuff.
d6946cd
to
b49a5e4
Compare
…roject view choice before performing sync calls.
dkashyn-sfdc
force-pushed
the
dynamic-qs-enablement
branch
from
b49a5e4
to
5bbf859
Compare
…roject view choice before performing sync calls.
Checklist
Please note that the maintainers will not be reviewing this change until all checkboxes are ticked. See
the Contributions section in the README for more
details.
Discussion thread for this change
Issue number:
7087Description of this change