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.

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

Restore file content changed #1005

Merged
merged 4 commits into from
Dec 4, 2018
Merged

Restore file content changed #1005

merged 4 commits into from
Dec 4, 2018

Conversation

pinussilvestrus
Copy link
Contributor

@pinussilvestrus pinussilvestrus commented Nov 14, 2018

  • restores the missing file content changed notification
  • rename App.js#tabHistory -> App.js#navigationHistory
  • refactor tab updating workarounds

Closes #970

@ghost ghost assigned pinussilvestrus Nov 14, 2018
@ghost ghost added the needs review Review pending label Nov 14, 2018
@philippfromme philippfromme requested review from nikku and removed request for philippfromme November 14, 2018 12:11
@pinussilvestrus pinussilvestrus changed the title 970 restore file content changed Restore file content changed Nov 14, 2018
@pinussilvestrus

This comment has been minimized.

@pinussilvestrus
Copy link
Contributor Author

pinussilvestrus commented Nov 20, 2018

Bug detected: When on XML-tab and agreeing the reloading, the changes did not affect. That might be caused by BpmnEditor#checkImport is not triggered again.

Fixed with last commit.

@pinussilvestrus pinussilvestrus force-pushed the 970-restore-file-changed branch from 52bc5ae to 9dda3bd Compare November 20, 2018 14:50
@nikku nikku force-pushed the 970-restore-file-changed branch from 9dda3bd to adfb1e2 Compare November 22, 2018 10:42
@ghost ghost assigned nikku Nov 22, 2018
Copy link
Member

@nikku nikku left a comment

Choose a reason for hiding this comment

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

We need to properly integrate the tab updating with the state management as sketched in 4335ce0.

Please update your implementation as discussed.

@nikku
Copy link
Member

nikku commented Nov 26, 2018

Rebased on latest next branch.

@pinussilvestrus pinussilvestrus force-pushed the 970-restore-file-changed branch 5 times, most recently from 89ec40c to a830a8d Compare November 28, 2018 11:05
@pinussilvestrus
Copy link
Contributor Author

Updated the implementation to fit with the wanted state management handling. For this there is a new method called updateTab(tab, newAttrs) which allows to update tab properties properly. It manage replacing the giving tab with a new tab + the new attributes.

I also renamed the label of the tabHistory to navigationHistory in the App.js component to indicate what this history-list is supposed to do: Saving which tabs were navigated to and when they were visited.

Now there are a couple of things to do:

When the second task is done I will give this PR to review again.

@philippfromme
Copy link
Contributor

I think the replacing of old namespaces one of the things that currently do not properly integrate with tab updating.

@pinussilvestrus
Copy link
Contributor Author

pinussilvestrus commented Nov 28, 2018

@philippfromme Indeed, this was also the first occurrence I was thinking about 👍

Another procedure would be the updating of an empty tab with the initial file contents in App.js#openEmptyFile. #1036 is kind of blocking it. #1037 is go to fix this.

@pinussilvestrus pinussilvestrus force-pushed the 970-restore-file-changed branch 2 times, most recently from e36dde4 to f74e679 Compare November 29, 2018 10:45
@pinussilvestrus pinussilvestrus force-pushed the 970-restore-file-changed branch 3 times, most recently from e8f116d to 4df85c0 Compare December 4, 2018 07:58
@pinussilvestrus
Copy link
Contributor Author

@nikku all changes should be adressed now!

@nikku nikku force-pushed the 970-restore-file-changed branch from 4df85c0 to 856a33f Compare December 4, 2018 08:17
Copy link
Member

@nikku nikku left a comment

Choose a reason for hiding this comment

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

👍

@nikku nikku merged commit a0b7c9d into next Dec 4, 2018
@ghost ghost removed the needs review Review pending label Dec 4, 2018
@delete-merged-branch delete-merged-branch bot deleted the 970-restore-file-changed branch December 4, 2018 08:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants