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

RHBA-415: After rename, designer is not marked as dirty more #275

Merged
merged 1 commit into from Apr 6, 2018
Merged

RHBA-415: After rename, designer is not marked as dirty more #275

merged 1 commit into from Apr 6, 2018

Conversation

karreiro
Copy link
Contributor

@karreiro karreiro commented Apr 4, 2018 •
edited

@karreiro karreiro requested review from manstis and jomarko April 4, 2018 19:35
@karreiro karreiro mentioned this pull request Apr 4, 2018
Merged
manstis
manstis reviewed Apr 4, 2018
View reviewed changes
Copy link
Member

@manstis manstis left a comment

Choose a reason for hiding this comment

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

@karreiro Presumably the issue was that the lock was not released when an asset was renamed? I'll review a bit more when I understand the issue.

@karreiro
Copy link
Contributor Author

karreiro commented Apr 4, 2018 •
edited

@mantis Precisely, Mike, that's the issue. The lock was not being released, and the editor was never marked as locked again.

There are two essential scenarios to cover:

The GDT Editor scenario:

 - Create a Guided Decision Table;
 - Rename the table;
 - Append some rows and add value to them;
 - (!) The editor was not being locked here.

Any other editor scenario (let's pick the DRL Editor):

 - Create a DRL file with the name "file";
 - Rename the DRL to "file1";
 - Make some changes in the file and save it;
 - Rename the DRL to "file2";
 - (!) The editor was not being locked and the title was not changing from "file1.drl - DRL" to "file2.drl - DRL".

manstis
manstis reviewed Apr 5, 2018
View reviewed changes
...mmons-editor-client/src/test/java/org/uberfire/ext/editor/commons/client/BaseEditorTest.java Outdated
final ObservablePath path = mock(ObservablePath.class);
final PlaceRequest placeRequest = mock(PlaceRequest.class);
final String version = "version";
final Callback<VersionRecord> callback = (v) -> {
Copy link
Member

Choose a reason for hiding this comment

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

Might have been more readable to simply use a mock Callback?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Fixed.

...rkbench/uberfire-workbench-client/src/main/java/org/uberfire/client/mvp/LockManagerImpl.java Outdated
private void updateLockInfo(@Observes LockInfo lockInfo) {
if (lockTarget != null && lockInfo.getFile().equals(lockTarget.getPath())) {
void updateLockInfo(final @Observes LockInfo lockInfo) {
if (getLockTarget() != null && lockInfo.getFile().toURI().equals(lockTarget.getPath().toURI())) {
Copy link
Member

Choose a reason for hiding this comment

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

You make a point of using toURI() on the paths here; but.....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@manstis Here we can have a PathImpl being compared to an ObservablePath (that's what we're avoiding), and......

...rkbench/uberfire-workbench-client/src/main/java/org/uberfire/client/mvp/LockManagerImpl.java
@@ -309,6 +321,12 @@ void onSaveInProgress(@Observes SaveInProgressEvent evt) {
}
}

void onRenameInProgress(@Observes RenameInProgressEvent event) {
if (getLockTarget() != null && event.getPath().equals(lockTarget.getPath())) {
Copy link
Member

Choose a reason for hiding this comment

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

... here you're OK using Path equality? (not URI, like your change above). Is this OK?

Copy link
Contributor Author

@karreiro karreiro Apr 5, 2018 •
edited

Choose a reason for hiding this comment

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

...here, we always have an ObservablePath being compared to another ObservablePath.

I added a comment to make this less confusing.

Thanks for the comment :-)

jomarko
jomarko reviewed Apr 5, 2018
View reviewed changes
Copy link

@jomarko jomarko left a comment

Choose a reason for hiding this comment

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

No comments to code, however I have question. Please follow my steps:

  • Pick some asset type, lets say plain drl
  • Create new asset of this type
  • do some changes (put some text)
  • click on rename, select save and rename, confirm
  • do some additional changes
  • again save the drl file
  • Check offered versions
    • Here is the question, is it expected that there are tracked versions just from the point of renaming? Shouldn't be there also versions before renaming? This is just question because not sure how it should work, please let me know.

@karreiro
Copy link
Contributor Author

karreiro commented Apr 5, 2018

@jomarko Good point. I'm not sure either. IMHO, the versions should be tracked right after every IO operation and not be lost.

Since it's not a regression, I think that this is not a blocker to this PR (but maybe we could raise a JIRA for that), see:

Before:
2018-04-05 6 46 45 pm

After:
2018-04-05 6 07 54 pm

jomarko
jomarko approved these changes Apr 6, 2018
View reviewed changes
Copy link

@jomarko jomarko left a comment •
edited

Choose a reason for hiding this comment

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

Thanks for comparing with state on master, discussed issue filed RHBA-732

@ederign ederign merged commit 2a04188 into kiegroup:master Apr 6, 2018
karreiro added a commit to karreiro/appformer that referenced this pull request Apr 9, 2018
@karreiro
RHBA-415: After rename, designer is not marked as dirty more (kiegrou…
ffe5d20 
…p#275)
@karreiro karreiro mentioned this pull request Apr 9, 2018
Merged
karreiro added a commit that referenced this pull request Apr 9, 2018
@karreiro
RHBA-415: After rename, designer is not marked as dirty more (#275) (#…
b4f5d3e 
…284)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@manstis manstis manstis approved these changes

@ederign ederign ederign approved these changes

@jomarko jomarko jomarko approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@karreiro @manstis @ederign @jomarko