Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

#6265 Target Management #6364

Open
wants to merge 56 commits into
base: master
Choose a base branch
from
Open

#6265 Target Management #6364

wants to merge 56 commits into from

Conversation

ahmedyarub
Copy link
Contributor

Add "Add Directory" button to the project view
Add directory toggle checkboxes to the project view

Checklist

  • I have filed an issue about this change and discussed potential changes with the maintainers.
  • I have received the approval from the maintainers to make this change.
  • This is not a stylistic, refactoring, or cleanup change.

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: 6265

Description of this change

Untitled Project

Add directory and management buttons to the project-view file. Note that this is the first part of a series of features and enhancements that target the hardships of navigating large repos using Bazel.

@ahmedyarub
bazelbuild#6265
bd3b270 
Add "Add Directory" button to the project view
Add directory toggle checkboxes to the project view
@ahmedyarub
Closes bazelbuild#6265
4fd8cb6 
Add target toggle checkboxes to the project view
@github-actions github-actions bot added product: CLion CLion plugin product: IntelliJ IntelliJ plugin product: GoLand GoLand plugin awaiting-review Awaiting review from Bazel team on PRs labels Apr 4, 2024
JamyDev
JamyDev reviewed Apr 4, 2024
View reviewed changes
Copy link
Contributor

@JamyDev JamyDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some very minor nits

base/src/com/google/idea/blaze/base/projectview/section/ListSection.java Outdated Show resolved Hide resolved
base/src/com/google/idea/blaze/base/projectview/section/ListSection.java Outdated Show resolved Hide resolved
...src/com/google/idea/blaze/base/projectview/section/sections/DirectoryLineMarkerProvider.java Outdated Show resolved Hide resolved
@ahmedyarub
Copy link
Contributor Author

Some very minor nits

Thank you @JamyDev. I think that now you'd need to accept the CLA otherwise the PR might not be mergeable :/ .

@ahmedyarub ahmedyarub force-pushed the ay/target_management branch 2 times, most recently from 9d24c62 to b4064ef Compare April 4, 2024 21:43
@tpasternak tpasternak linked an issue Apr 5, 2024 that may be closed by this pull request
Target Manager #6265
Open
tpasternak
tpasternak requested changes Apr 5, 2024
View reviewed changes
Copy link
Collaborator

@tpasternak tpasternak left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, thank you for the contribution. I've noticed a few issues:

  1. The behavior of the checkbox appears unpredictable when there are duplicates in the list (refer to the video).
  2. The "Project" tool window on the left-hand side updates with each click, but its behavior is also unpredictable (refer to the video).
  3. The case when there are two separate "directories" sections is not covered at all (same for targets)
  4. The + (add directory) button is barely useful. It's still more convenient to add a folder directly, as you get completions there. I would suggest removing it for now and revisiting this feature later.
  5. Please update copyright year
checkboxes.mov

@ahmedyarub
Copy link
Contributor Author

Hi, thank you for the contribution. I've noticed a few issues:

  1. The behavior of the checkbox appears unpredictable when there are duplicates in the list (refer to the video).
  2. The "Project" tool window on the left-hand side updates with each click, but its behavior is also unpredictable (refer to the video).
  3. The case when there are two separate "directories" sections is not covered at all (same for targets)
  4. The + (add directory) button is barely useful. It's still more convenient to add a folder directly, as you get completions there. I would suggest removing it for now and revisiting this feature later.
  5. Please update copyright year
  1. I'll test and fix that today. But are duplicate directories allowed?
  2. What do you mean by "unpredictable" here exactly? And which video are you referring to exactly? The one I uploaded?
  3. That I'll fix but I did notice that the plugin casually picks up "the last directories tag" so I thought that I would do the same. I'll still fix it not a big deal really.
  4. I'll put it behind an experiment that is disabled by default. This button will have a much larger role later as I mentioned in the issue. But for now let's hide it for everybody else, yea.
  5. We changed it to 2024 yesterday. Should I change that?

@tpasternak
Copy link
Collaborator

oh, regarding 1 and 2 - I just forgot to upload the video, please see the message again #6364 (review)

@ahmedyarub
Copy link
Contributor Author

@tpasternak is my version of the plugin being used here? I can't see any checkboxes.

@tpasternak
Copy link
Collaborator

oh, my fault, this was supposed to be uploaded to a different PR xd

@ahmedyarub
Copy link
Contributor Author

oh, my fault, this was supposed to be uploaded to a different PR xd

Happy Friday 🍺

@tpasternak
Copy link
Collaborator

Now it's the right one

@ahmedyarub
Copy link
Contributor Author

OK then regarding 2: it looks like you have enabled a directory/target that hasn't been synchronized before, right? I have already considered that case and I was thinking of adding a notification in a future version tell the user that some of enabled/added targets are still not synched. The notification would be just a floating button just like that Maven refresh one if you have seen it before. What do you think?
In fact, I was considering implementing that as a part of the (Partially sync only the un-synced targets) feature.
My questions 1 and 5 still stand.

@ahmedyarub
Copy link
Contributor Author

@tpasternak fixed all the points above. Note that this PR now depends on the one that adds line numbers.

Ahmed Yarub Hani Al Nuaimi and others added 25 commits April 22, 2024 21:08
bazelbuild#6370
0ecd5bd 
Handle targets with colons
@ahmedyarub @blorente
Update aswb/src/com/google/idea/blaze/android/projectview/AndroidSdkP…
f409635 
…latformSection.java

Co-authored-by: Borja Lorente <[email protected]>
@ahmedyarub @blorente
Update aswb/src/com/google/idea/blaze/android/projectview/AndroidSdkP…
ba6bd57 
…latformSection.java

Co-authored-by: Borja Lorente <[email protected]>
@ahmedyarub @blorente
Update aswb/src/com/google/idea/blaze/android/projectview/AndroidSdkP…
9b0d15b 
…latformSection.java

Co-authored-by: Borja Lorente <[email protected]>
@ahmedyarub @blorente
Update aswb/src/com/google/idea/blaze/android/projectview/AndroidSdkP…
0f710f9 
…latformSection.java

Co-authored-by: Borja Lorente <[email protected]>
@ahmedyarub @blorente
Update base/src/com/google/idea/blaze/base/projectview/ProjectView.java
16161ba 
Co-authored-by: Borja Lorente <[email protected]>
@ahmedyarub @blorente
Update base/src/com/google/idea/blaze/base/projectview/section/sectio…
111957c 
…ns/DirectorySection.java

Co-authored-by: Borja Lorente <[email protected]>
@ahmedyarub @blorente
Update base/src/com/google/idea/blaze/base/projectview/section/sectio…
3b96874 
…ns/AutomaticallyDeriveTargetsSection.java

Co-authored-by: Borja Lorente <[email protected]>
bazelbuild#6370
f63dd53 
Remove line calculation from print() methods
bazelbuild#6370
9cd006a 
Remove line calculation from print() methods
Merge remote-tracking branch 'ahmedyarub/ay/section_line_number' into…
66a0f50 
… ay/section_line_number
Merge branch 'refs/heads/ay/section_line_number' into ay/target_manag…
3891093 
…ement
bazelbuild#6265
df09fc3 
Remove redundant method
bazelbuild#6370
a28e51f 
Remove redundant method
Merge branch 'refs/heads/ay/section_line_number' into ay/target_manag…
bf28bc9 
…ement
bazelbuild#6265
057685f 
Readd required method
bazelbuild#6265
9aa305e 
Update comment
bazelbuild#6265
b506482 
Update comment
Merge branch 'master' of https://github.com/bazelbuild/intellij into …
3d4299a 
…ay/target_management
@ahmedyarub
Merge branch 'master' into ay/section_line_number
2302d6a
@ahmedyarub
Merge branch 'master' into ay/target_management
a92a4a4
Merge branch 'ay/target_management' of https://github.com/ahmedyarub/…
65971e0 
…intellij into ay/target_management
@ahmedyarub @blorente
Update aswb/src/com/google/idea/blaze/android/projectview/AndroidSdkP…
64d085f 
…latformSection.java

Co-authored-by: Borja Lorente <[email protected]>
@ahmedyarub
Merge branch 'refs/heads/ay/section_line_number' into ay/target_manag…
10f28d4 
…ement
@ahmedyarub
Merge branch 'master' of https://github.com/bazelbuild/intellij into …
90376a2 
…ay/target_management

# Conflicts:
#	base/src/com/google/idea/blaze/base/projectview/section/ListSection.java
@ahmedyarub
Copy link
Contributor Author

@tpasternak rebased and ready for review 😄

@ahmedyarub
Copy link
Contributor Author

Looks like the CLion test failed due to an unrelated HTTP error 😅

@ahmedyarub
Copy link
Contributor Author

Hi @tpasternak. Is there any chance for this to be merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JamyDev JamyDev JamyDev left review comments

@tpasternak tpasternak tpasternak requested changes

@mai93 mai93 Awaiting requested review from mai93 mai93 is a code owner

@jastice jastice Awaiting requested review from jastice

@agluszak agluszak Awaiting requested review from agluszak agluszak is a code owner

Requested changes must be addressed to merge this pull request.

Assignees

@tpasternak tpasternak

@blorente blorente

@mai93 mai93

Labels
awaiting-review Awaiting review from Bazel team on PRs product: CLion CLion plugin product: GoLand GoLand plugin product: IntelliJ IntelliJ plugin
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Target Manager
5 participants
@ahmedyarub @tpasternak @JamyDev @blorente @mai93