Skip to content

Commit

Permalink
Fix bug #20
Browse files Browse the repository at this point in the history
  • Loading branch information
@groboclown
groboclown committed Feb 18, 2015
1 parent 4624bb3 commit 13109d7
Show file tree
Hide file tree
Showing 13 changed files with 67 additions and 333 deletions.
11 changes: 11 additions & 0 deletions CHANGES.md
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,16 @@
# IDEA Community VCS Integration for Perforce

## ::v0.3.2::

### Overview

* Minor UI bug fixes.

### Details

* Minor UI bug fixes.
* Returning online can cause error "user selected work offline" (#20)

## ::v0.3.1::

### Overview
Expand Down
287 changes: 2 additions & 285 deletions README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ This is the location for the Perforce VCS integration into the [IntelliJ IDEA Co

**Currently Supported IDEA versions: 13.5 (Android Studio), 14.0 (build 14.2118 and above)**

*Other versions within that IDEA build range may work, but they haven't been tested.*


# Getting Started

Expand Down Expand Up @@ -142,288 +144,3 @@ revisions against each other.

Currently, submitting a changelist is not enabled.


# Status and Todo


## Functional Areas and their Problems


### Audit Code and Refactoring

* Investigate all remaining "FIXME" and "TODO" items
* Reduce the amount of LOG.info usage.


### Connection Widget

* The "did connect" message is sent too early, which means the
widget still shows "disconnected" even when it's running in
connected mode. Clicking on the widget refreshes the
status correctly.
**Minor**


### Submit

* Submit is currently disabled, because it crashes Windows P4 servers.
*Major Feature*


### Connection Settings

* Add in Trust Ticket to connection gui.
**Minor**
* Selecting "Relative P4CONFIG" should ensure the client load button is really
disabled. It currently only sometimes works.
**Minor**


### Diff

* The changelist-based "Ctrl-D" diff shows a difference against *head* revision, not *have*.
This should probably be an option.


### Revert

* Reverting a moved file requires a project file refresh to
see it moved into the correct location.
**Minor**
* Has special code to handle the moved/renamed file pairs
(reverting one end of the move/rename reverts the other
one, too).


### Changelist

* A file which is edited but not checked out is not processed
correctly. It seems to always become checked out.
**Minor**


### Move a file between changelists

* Move/add and move/delete files are both put into the moved changelist
when only one is moved. However, this is only reflected in the UI when the
changelist view is refreshed (the changelist view only shows one file moved).
**Minor**


### Add/Edit

* If a file was edited without telling VCS (edit during offline mode, etc),
then the server is reconnected and the file is added (Alt+Shift+A),
the file is not reflected in the correct changelist. A refresh is needed
to show it. The file cannot be reverted until the refresh happens.
**Minor**


### Rename / Move

* Moving a file will correctly show the old/new files in the changelist, but
it can also incorrectly mark the directory and other files as "Modified without checkout".
Refreshing the changelist fixes this, but adding that to the code (P4VCSListener) seems to
invoke it too early; there's something triggering the dirty flag after the move listener
fires.
**Minor**
* Need to test out moving a file that has already been:
* edited - has a problem; keeps original file open for edit.
p4v works by opening the file for edit (even if it's already open for edit),
then performing the move operation.
* moved - p4v works by opening the initially renamed file for edit, then
running the move on the initial renamed file to the new name.
* added

P4V seems to do the exact same procedure for all of these operations -
open for edit then move.

P4V reports an error if you move a file to a file that already exists with
the same name.

P4V reports an error if you delete a file, then move a file to be the same
name as the just-deleted file. You need to submit the delete first.

Fortunately, IDEA keeps the deleted file in the changelist.


### Copy

* Need to retest these scenarios:
* target is deleted on depot, not on client
* target is open for edit or add
* Copying a file that is currently opened for integrate or move will just
add a new file, and not make it be integrated from the underlying source.
*This only matters if the `use integration` flag is set, which currently can't be set.*
Once `use integration` flag is enabled, this will be a **Minor** issue.


### Errors

* Errors need to be localized
**Minor**


### VCS Popup

* Works fine.


### History

* Seems to work fine.


### Delete

* Delete seems to work.


### Synchronize a file

* Seems to work fine.


## Improvements

### General

* Proper P4IGNORE support. The VCS itself provides some level of API support for this, but it will need to be
integrated deeper into the calls.


### Checkout

* "Checkout" provider is essentially the sync in Perforce terms. Implement this.
* Add sync (checkout) to popup menu.


### UI Changelists

* Add Job add/remove components to the changelist modify dialog.
* On submit, allow for setting jobs and job status. See TodoCheckinHandler for
how to get UI elements in the checkin handler. Although,
P4OnCheckinPanel looks to be where it should be.
* Look at the com.intellij.openapi.vcs.changes.ui.NewChangelistDialog class to see where
the new changelist dialog is instantiated. Doesn't look like it can be extended
to add new options. May need to just replace the corresponding action
(com.intellij.openapi.vcs.changes.actions.AddChangeListAction), which is
defined in the VcsActions.xml file with id "ChangesView.NewChangeList".
Replace the whole "ChangesViewToolbar" group?


### UI File Icon

* Mark files in the project folder with perforce status - does it need
resolving? Is it out of date? Text color already shows the general
add/edit/move status.
* StructureViewTreeElement. The Vcs apparently plugs into this to show
the lock icon.
* The P4StatusUpdateEnvironment looks like it should be handling this,
but it never seems to be invoked.


### History

* Add full getVcsBlockHistoryProvider impls.
* P4CommittedChangesProvider.java needs implemented methods.


### Connection Settings

* Look into handling the P4ENVIRO file.
* Support SSO


### Integrate on Copy

* There's a flag that allows for selecting whether the user wants to
perform an integrate on copy or just an add. Right now, this is
hard coded to "add", but can be selectable in the future. This
might be an option that can be added in the copy dialog.


### Hacks

* P4CheckinEnvironment isn't correctly being returned by
P4Vcs.getCheckinEnvironment.
A hack is in place to make it work, but it needs a proper fix.
* Same thing with P4RollbackEnvironment.
* BackgroundTask should reference the VcsConfiguration to determine which task
is run in the background.
* Support for files with wildcard characters (#@%\*) works, but is kind of
messy. It's currently contained in FileSpecUtil, and P4FileInfo, and
a bit of really awful functionality in P4Exec.addFiles.


### Tests

* Add unit tests


# Code Style

## Exceptions

In general, only `VcsException` and `CancellableException` should be thrown.
The only place the deeper `P4JavaException` and family are thrown is in the
`P4Exec` class, and that's only when wrapped in the p4RunFor call.


## FilePath

`FilePath.getPath()` usually returns the value for the contained `VirtualFile`; however, for
a FilePath that references the source of a moved file (deleted), it will return the
moved-to destination. Looks like an IDEA bug. To avoid this, I add this to
all such calls:

// Warning: for deleted files, fp.getPath() can be different than the actual file!!!!
// use this instead: getIOFile().getAbsolutePath()

Also, try to block directories from going into Perforce file specs. The only place
the code works with directories is finding open files.


## Threading

Without special care, deadlocks are common. Never call "invokeAndWait"
from the EDT thread. On the rarest occasion that you need a result
from a user, and the code can be run from the EDT or a background thread,
then use a VcsFutureSetter and its "runInEdt" call to make sure the
right, non-deadlock code is invoked.

Also, any bit of code that is synchronized cannot call out to the EDT,
unless it is carefully placed in a background thread.
This means no invoking P4 calls in a synchronized block unless it's in
a background thread. Be careful here! There are some parts of the
`VcsUtil` that obtain a vcs write lock that can block.


## Connection Status

Connection status must be carefully handled to avoid deadlock problems.

Status changes on connection (connected or disconnected) are run through the MessageBus.


## Handling FileSpecs

Do not use the P4 Java API class `FileSpecBuilder`. Instead, use the `P4FileInfo`
where possible, and when needing to get deep into the Perforce interaction, use the
`FileSpecUtil`. The *ONLY* exception is when dealing with "p4 add", because that
has special usage.


# P4Java API bugs

* p4 submit -i crashes Windows servers, most likely because the incoming
submit map is wrong. Although I believe this also happens when this is
changed to pass everything as arguments rather than the input map.
*Need to retest now that the P4Exec is under better control, but it
should still be re-implemented to better conform to the p4d requirements.*
* Looks like passing the password in the property doesn't work right.
It still needs a "login", but at least we can not store the login
in the ticket file.
* p4java API can flood the temp directory with "p4j\*.tmp" files.
The plugin has a watch-dog thread that cleans these up
periodically.
7 changes: 6 additions & 1 deletion plugin/META-INF/plugin.xml
View file
Original file line number Diff line number Diff line change
@@ -1,11 +1,16 @@
<idea-plugin version="2">
<name>Perforce IDEA Community Integration</name>
<id>PerforceIC</id>
<version>0.3.1.0</version>
<version>0.3.1.1</version>
<idea-version since-build="IC-135.1286"/>
<category>VCS Integration</category>
<change-notes><![CDATA[
<ol>
<li><em>0.3.2</em>
<ol>
<li>Minor UI bug fixes.</li>
</ol>
</li>
<li><em>0.3.1</em>
<ol>
<li>Add support for IDEA 135 builds.</li>
Expand Down
32 changes: 3 additions & 29 deletions plugin/src/net/groboclown/idea/p4ic/actions/P4WorkOnlineAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
import net.groboclown.idea.p4ic.config.ServerConfig;
import net.groboclown.idea.p4ic.extension.P4Vcs;
import net.groboclown.idea.p4ic.server.ServerStoreService;
import net.groboclown.idea.p4ic.server.exceptions.P4InvalidConfigException;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;

Expand All @@ -44,36 +43,11 @@ public void actionPerformed(@NotNull AnActionEvent e) {
final P4Vcs vcs = P4Vcs.getInstance(getProject(e));
Set<ServerConfig> offlineServers = new HashSet<ServerConfig>();

// Tell the servers that they should be working online.
ServerStoreService.getInstance().workOnline();

// Force the configurations to be reloaded.
vcs.reloadConfigs();

for (Client client : vcs.getClients()) {
if (client.isWorkingOffline()) {
offlineServers.add(client.getConfig());
} else {
// check if it really is online
try {
client.getServer().checkConnection();
} catch (P4InvalidConfigException ex) {
// Should be correctly handled by the thrower
//ErrorDialog.logError(project, P4Bundle.message("configuration.dialog.valid-connection.title"), ex);
// offlineServers.add(client.getConfig());
}
}
}

for (ServerConfig config: offlineServers) {
while (true) {
try {
ServerStoreService.getInstance().getServerStatus(project, config).
onReconnect();
break;
} catch (P4InvalidConfigException e1) {
// should be handled by the thrower
// FIXME at least log the issue
}
}
}
}

/**
Expand Down
Loading

0 comments on commit 13109d7

Please sign in to comment.