show package version information in Files View

show package version information in Files View
Committed: https://code.google.com/p/dart/source/detail?r=16580

[keertip, 2013-01-02 10:54:18]

- builder responds to changes in pubspec.lock
- store package version info as resource property
- listener for pub updates
- show version info as decorator in files view

[danrubel, 2013-01-02 13:41:33]

https://codereview.chromium.org/11735003/diff/3001/editor/tools/plugins/com.g...
File
editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/pub/PubBuildParticipant.java
(right):

https://codereview.chromium.org/11735003/diff/3001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/pub/PubBuildParticipant.java:104:
IResource folder = project.findMember(DartCore.PACKAGES_DIRECTORY_NAME +
File.separator
This won't work for pubspec files located in a child folder. I recommend not
passing in project, and changing this line to 

lockFile.getParent().findMember(...)

https://codereview.chromium.org/11735003/diff/3001/editor/tools/plugins/com.g...
File
editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/utilities/yaml/PubYamlUtils.java
(right):

https://codereview.chromium.org/11735003/diff/3001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/utilities/yaml/PubYamlUtils.java:140:
public static Map<String, String> getPackageVersionMap(String lockFileContents)
{
We need a test for this so that if pub changes the lock file structure, then it
fails our test so that we know to update this code.

https://codereview.chromium.org/11735003/diff/3001/editor/tools/plugins/com.g...
File
editor/tools/plugins/com.google.dart.tools.core_test/src/com/google/dart/tools/core/pub/PubBuildParticipantTest.java
(right):

https://codereview.chromium.org/11735003/diff/3001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.core_test/src/com/google/dart/tools/core/pub/PubBuildParticipantTest.java:49:
private ArrayList<IContainer> actual = new ArrayList<IContainer>();
Per comment below, perhaps removing this field in favor of
IContainer runPubContainer
and
IFile lockFileProcessed

https://codereview.chromium.org/11735003/diff/3001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.core_test/src/com/google/dart/tools/core/pub/PubBuildParticipantTest.java:82:
actual.add(project);
This won't be able to assert the difference between a call to runPub vs
processLockFileContents. We only test if it has been called once or not at all,
so perhaps changing assertCalls to assertRunPub(IContainer/null) and
assertProcessLockFileContents(IContainer/null)

https://codereview.chromium.org/11735003/diff/3001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.core_test/src/com/google/dart/tools/core/pub/PubBuildParticipantTest.java:113:
public void test_build_full_pub_lockFile() throws Exception {
Please add a test where the pubspec lock file is a sibling to a pubspec.yaml
file that is not located in the project root

https://codereview.chromium.org/11735003/diff/3001/editor/tools/plugins/com.g...
File
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/filesview/FilesView.java
(right):

https://codereview.chromium.org/11735003/diff/3001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/filesview/FilesView.java:137:
treeViewer.refresh(container);
Does the entire container need to be refreshed? 
Or can we update one file or one aspect of the container?

[keertip, 2013-01-02 15:24:27]

https://codereview.chromium.org/11735003/diff/3001/editor/tools/plugins/com.g...
File
editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/pub/PubBuildParticipant.java
(right):

https://codereview.chromium.org/11735003/diff/3001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/pub/PubBuildParticipant.java:104:
IResource folder = project.findMember(DartCore.PACKAGES_DIRECTORY_NAME +
File.separator
On 2013/01/02 13:41:33, danrubel wrote:
> This won't work for pubspec files located in a child folder. I recommend not
> passing in project, and changing this line to 
> 
> lockFile.getParent().findMember(...)

Done.

https://codereview.chromium.org/11735003/diff/3001/editor/tools/plugins/com.g...
File
editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/utilities/yaml/PubYamlUtils.java
(right):

https://codereview.chromium.org/11735003/diff/3001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/utilities/yaml/PubYamlUtils.java:140:
public static Map<String, String> getPackageVersionMap(String lockFileContents)
{
Agreed. Will look into this and see what can be done.

On 2013/01/02 13:41:33, danrubel wrote:
> We need a test for this so that if pub changes the lock file structure, then
it
> fails our test so that we know to update this code.

https://codereview.chromium.org/11735003/diff/3001/editor/tools/plugins/com.g...
File
editor/tools/plugins/com.google.dart.tools.core_test/src/com/google/dart/tools/core/pub/PubBuildParticipantTest.java
(right):

https://codereview.chromium.org/11735003/diff/3001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.core_test/src/com/google/dart/tools/core/pub/PubBuildParticipantTest.java:49:
private ArrayList<IContainer> actual = new ArrayList<IContainer>();
On 2013/01/02 13:41:33, danrubel wrote:
> Per comment below, perhaps removing this field in favor of
> IContainer runPubContainer
> and
> IFile lockFileProcessed

Done.

https://codereview.chromium.org/11735003/diff/3001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.core_test/src/com/google/dart/tools/core/pub/PubBuildParticipantTest.java:82:
actual.add(project);
On 2013/01/02 13:41:33, danrubel wrote:
> This won't be able to assert the difference between a call to runPub vs
> processLockFileContents. We only test if it has been called once or not at
all,
> so perhaps changing assertCalls to assertRunPub(IContainer/null) and
> assertProcessLockFileContents(IContainer/null)

Done.

https://codereview.chromium.org/11735003/diff/3001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.core_test/src/com/google/dart/tools/core/pub/PubBuildParticipantTest.java:113:
public void test_build_full_pub_lockFile() throws Exception {
On 2013/01/02 13:41:33, danrubel wrote:
> Please add a test where the pubspec lock file is a sibling to a pubspec.yaml
> file that is not located in the project root

Done.

https://codereview.chromium.org/11735003/diff/3001/editor/tools/plugins/com.g...
File
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/filesview/FilesView.java
(right):

https://codereview.chromium.org/11735003/diff/3001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/filesview/FilesView.java:137:
treeViewer.refresh(container);
Changed to refresh only the relevant packages directory.

On 2013/01/02 13:41:33, danrubel wrote:
> Does the entire container need to be refreshed? 
> Or can we update one file or one aspect of the container?

[danrubel, 2013-01-02 17:21:55]

lgtm

https://codereview.chromium.org/11735003/diff/8001/editor/tools/plugins/com.g...
File
editor/tools/plugins/com.google.dart.tools.core_test/src/com/google/dart/tools/core/pub/PubBuildParticipantTest.java
(right):

https://codereview.chromium.org/11735003/diff/8001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.core_test/src/com/google/dart/tools/core/pub/PubBuildParticipantTest.java:213:
target.assertProcessLockFile(file);
Need to add target.assertRunPub(null) here and other places
assertProcessLockFile(...) is called, and add assertProcessLockFile(null)
wherever assertRunPub(...) is called.

[devoncarew, 2013-01-02 17:28:20]

lgtm

https://chromiumcodereview.appspot.com/11735003/diff/8001/editor/tools/plugin...
File
editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/pub/PubBuildParticipant.java
(right):

https://chromiumcodereview.appspot.com/11735003/diff/8001/editor/tools/plugin...
editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/pub/PubBuildParticipant.java:105:
DartCore.PACKAGES_DIRECTORY_NAME + File.separator + key);
Instead of File.separator I think we want "/". I believe the resource system
always uses forward slashes.

[keertip, 2013-01-02 17:31:38]

https://codereview.chromium.org/11735003/diff/8001/editor/tools/plugins/com.g...
File
editor/tools/plugins/com.google.dart.tools.core_test/src/com/google/dart/tools/core/pub/PubBuildParticipantTest.java
(right):

https://codereview.chromium.org/11735003/diff/8001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.core_test/src/com/google/dart/tools/core/pub/PubBuildParticipantTest.java:213:
target.assertProcessLockFile(file);
On 2013/01/02 17:21:55, danrubel wrote:
> Need to add target.assertRunPub(null) here and other places
> assertProcessLockFile(...) is called, and add assertProcessLockFile(null)
> wherever assertRunPub(...) is called.

Done.

[keertip, 2013-01-02 17:43:16]

https://chromiumcodereview.appspot.com/11735003/diff/8001/editor/tools/plugin...
File
editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/pub/PubBuildParticipant.java
(right):

https://chromiumcodereview.appspot.com/11735003/diff/8001/editor/tools/plugin...
editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/pub/PubBuildParticipant.java:105:
DartCore.PACKAGES_DIRECTORY_NAME + File.separator + key);
On 2013/01/02 17:28:20, devoncarew wrote:
> Instead of File.separator I think we want "/". I believe the resource system
> always uses forward slashes.

Done.