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

Source tab entry duplication #3360

Merged
merged 6 commits into from
Oct 27, 2017
Merged

Source tab entry duplication #3360

merged 6 commits into from
Oct 27, 2017

Conversation

lenhard
Copy link
Member

@lenhard lenhard commented Oct 27, 2017

Adapts the source tab a little better to the new and no longer re-created entry editor. It is a fix for the problem described in this comment #3352 (comment) The remaining problems in the issue are not addressed here, so the issue should stay open when merging this and the update flow in the source tab should be reworked in further PRs.

  • Change in CHANGELOG.md described
  • Tests created for changes
  • Screenshots added (for bigger UI changes)
  • Manually tested changed features in running JabRef
  • Check documentation status (Issue created for outdated help page at help.jabref.org?)
  • If you changed the localization: Did you run gradle localizationUpdate?

@halirutan
Copy link
Collaborator

halirutan commented Oct 27, 2017

To give some feedback and to spare you unnecessary work. I had already rewritten this yesterday and I'm sorry for not saying that I look over this. When I saw that you started this too, I fetched your branch and my initial idea was, just to provide the functionality to make Ctrl+S work.

However, I believe the information flow for the EntryEditor is extremely confusing so I ended up rewriting this completely. The main idea is to make the EntryEditorimplement EntryEditorInfo that provides access for each tab. Therefore, we don't pass entries, entry-types, contexts, etc. between the editor and its tabs, instead, each tab accesses the information when it needs it. This let me through out all the methods that pass things and the tabs are only notified when they get focus or when a new entry is set by the EntryEditor. This makes things much more localized and easier to follow.

I would like to push this on a new branch and test it carefully. It will include the changes you made here. The issue with Ctrl+S can be resolved by putting this in the setupKeyBindings() for the EntryEditor where we act on (but don't consume) that save/saveAll events:

switch (keyBinding.get()) {             
    case CUT:                           
    case COPY:                          
    case PASTE:                         
    case CLOSE_ENTRY_EDITOR:            
    case DELETE_ENTRY:                  
    case SELECT_ALL:                    
    case ENTRY_EDITOR_NEXT_PANEL:       
    case ENTRY_EDITOR_NEXT_PANEL_2:     
    case ENTRY_EDITOR_PREVIOUS_PANEL:   
    case ENTRY_EDITOR_PREVIOUS_PANEL_2: 
        e.consume();                    
        break;                          
    case SAVE_ALL:                      
    case SAVE_DATABASE:                 
    case SAVE_DATABASE_AS:              
        sourceTab.saveCurrentSource();  
        break;                          
    default:                            
        //do nothing                    
}                                       

The things I tested worked but there is one complicated situation: User edited the bibtex source and made a syntax error in it. Then he clicks on a different entry in the main table. Besides that the current "let me edit my wrong code" does not work as it stands (exception is not caught), there is a deeper problem: The main table is already on a different entry and the EntryEditor has already fetched the new entry. To make a successful revert&edit we need to tell the main table to jump back to the erroneous entry and set it in EntryEditor.

@lenhard Shall I put my changes on a different branch? In this way we have a quick fix with your PR and can test things carefully.

@lenhard
Copy link
Member Author

lenhard commented Oct 27, 2017

@halirutan Thanks for your input! What you write sounds reasonable and I agree that the information flow in the entry editor is crying for a larger refactoring.

I have just pushed a fix to repair the build here and when it has passed we could merge this already. You can do your changes on a different branch, then. And we can give it a thorough look and testing.

Just be aware that even more problems apart from syntax error correction might come up. The tabs are not only dependent on the entry editor, but also partly dependent on each other. E.g. if a field value changes in the source tab, it should be changed in another tab as well and vice versa (not so hard) and if I add a new optional field in the source tab, it should make the optional tab make appear (somewhat harder).

if (codeArea != null && codeArea.focusedProperty().get()) {
DefaultTaskExecutor.runInJavaFXThread(() -> storeSource());
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

The problem here is btw that you run it in a separate thread. This means, the moment storeSource starts to work, a new entry is already set and what you do is to overwrite the correct new entry with itself.

@halirutan
Copy link
Collaborator

@lenhard

if I add a new optional field in the source tab, it should make the optional tab make appear (somewhat harder).

yes, even this works already.

@lenhard
Copy link
Member Author

lenhard commented Oct 27, 2017

Perfect! Then I'll merge this directly so that we have a quick fix to the issue and you can go ahead with your larger refactoring.

@lenhard lenhard merged commit 6bb21d4 into master Oct 27, 2017
@lenhard lenhard deleted the source-tab-duplication branch October 27, 2017 13:52
Siedlerchr added a commit that referenced this pull request Oct 27, 2017
* upstream/master: (31 commits)
  Source tab entry duplication (#3360)
  Use CITE_COMMENT not only for external latex editors but also for cop… (#3351)
  Updating with new translations (#3348)
  Upgrade error-prone (#3350)
  Jabref_pt_BR partially updated (#3349)
  Used late initialization for context menus (#3340)
  Fix NPE when calling with bib file as cmd argument (#3343)
  update mockito-core from 2.10.0 -> 2.11.0 (#3338)
  Remove underscore in Localized messages (#3336)
  Localisation: French: new entries translated (#3337)
  Refactoring: Lazy init of all editor tabs (#3333)
  Initializing EntryEditor Tabs on focus (#3331)
  Fix #3292: annotations are now automatically refreshed (#3325)
  Change integrity message for names depending on database mode (#3330)
  Fix location bundle with fast access (#3327)
  Small code cleanup
  Fix "path not found" exception
  Small fix in drag and drop handler of linked files (#3328)
  Fix NPE in MainTable (#3318)
  Increase relative size of abstract field in editor (#3320)
  ...
Siedlerchr added a commit that referenced this pull request Oct 31, 2017
* upstream/master:
  Rework AutoSetFileLinks (#3368)
  revert wrongly commited changes
  Refactorings (#3370)
  Fix freezing when running cleanup file operations like rename (#3315)
  This additional style setting for IDEA (#3355)
  Fix checkstyle
  Add proper equals to content selector
  Source tab entry duplication (#3360)
  Use CITE_COMMENT not only for external latex editors but also for cop… (#3351)
  Updating with new translations (#3348)
  Upgrade error-prone (#3350)
  Jabref_pt_BR partially updated (#3349)
  Delete unused code
  Extract difference finder from gui to logic
  Used late initialization for context menus (#3340)
  Remove unnecessary EntrySorter
  Move existing code to gui and rename change classes to view model
  Small code improvements
Siedlerchr added a commit that referenced this pull request Nov 1, 2017
* upstream/master: (79 commits)
  Update richtext and flowless (#3386)
  Source tab: hide notification pane when code is correct again (#3387)
  Titles are optional at crossref (#3378)
  Update entry for every change in the source panel (#3366)
  Rework AutoSetFileLinks (#3368)
  revert wrongly commited changes
  Refactorings (#3370)
  Fix freezing when running cleanup file operations like rename (#3315)
  This additional style setting for IDEA (#3355)
  Fix checkstyle
  Add proper equals to content selector
  Source tab entry duplication (#3360)
  Use CITE_COMMENT not only for external latex editors but also for cop… (#3351)
  Updating with new translations (#3348)
  Upgrade error-prone (#3350)
  Jabref_pt_BR partially updated (#3349)
  Delete unused code
  Extract difference finder from gui to logic
  Used late initialization for context menus (#3340)
  Fix NPE when calling with bib file as cmd argument (#3343)
  ...
Siedlerchr added a commit that referenced this pull request Nov 3, 2017
* upstream/master:
  Source tab entry duplication (#3360)
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.

2 participants