Skip to content

Conversation

@Cesar-Medeiros
Copy link

@Cesar-Medeiros Cesar-Medeiros commented Dec 19, 2018

Description

Enabling the user to open folders containing files, or files themselves, created by the application.

Open file

Issue fixed

#2408

Type of changes

  • ⚪ Bug fix (Change that fixed an issue)
  • ⚪ Breaking change (Change that can cause existing functionality to change)
  • ⚪ Improvement (Change that improves the code. Maybe performance or development improvement)
  • 🔘 Feature (Change that adds new functionality)
  • ⚪ Documentation change (Change that modifies documentation. Maybe typo fixes)

Checklist:

  • 🔘 My code follows the project code style
  • 🔘 I have written test for my code and it has been tested
  • 🔘 All existing tests have been passed
  • 🔘 I have attached a screenshot/video to visualize my change if possible

@MargaridaSilva
Copy link

We would really appreciate to know whether this PR is ready to be accepted, hence we are working on this open source project in the context of a university course and it would be beneficial to discuss this PR, or even see it being approved.

@ZeroX-DG ZeroX-DG added the awaiting review ❇️ Pull request is awaiting a review. label Dec 20, 2018
@ZeroX-DG
Copy link
Member

ZeroX-DG commented Dec 20, 2018

@margarida-silva I'll review and approve this PR soon for your uni course so if you can provide me with more information about this PR I'll be able to speed up the review process. I don't know how much time you can wait for this PR to be approved so please don't hesitate to tell me if you need this PR to be reviewed faster.

@Cesar-Medeiros
Copy link
Author

@ZeroX-DG Thanks for your reply. In this PR we added the functionality of opening a file instead of just importing it. Each time a file is opened by clicking on "Open File" we can change the note in boostnote and the original file is also modified. The same happens if we edit the original file, the note will be modified in boostnote. The gif attached in the main post helps to understand this PR.

Thanks for speeding up the review process.

Copy link
Member

@ZeroX-DG ZeroX-DG left a comment

Choose a reason for hiding this comment

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

This is a nice feature to have. Please update your code 👍 I'll try to finish reviewing this before new year 😄

const note = this.notes[0]
const prevKey = prevProps.location.query.key
const noteKey = visibleNoteKeys.includes(prevKey) ? prevKey : note && note.key
const CSON = require('@rokt33r/season')
Copy link
Member

Choose a reason for hiding this comment

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

Can you move this to the top of the file?

if (focusNote.filepath !== undefined) {
try {
this.fsWatcher.close()
} catch (e) {
Copy link
Member

Choose a reason for hiding this comment

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

Please at least console.error the error. It might come in handy in the future.


case 'rename':
updatedNote.filepath = undefined
this.fsWatcher.close()
Copy link
Member

Choose a reason for hiding this comment

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

Why did you have to catch for errors in the previous line 150 and not here?

Copy link
Author

Choose a reason for hiding this comment

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

I forgot to add exception handling. I just updated the code with all suggestions.

@ZeroX-DG
Copy link
Member

ZeroX-DG commented Jan 4, 2019

Please fix the conflict and also I encounter this error when I run the app.
screenshot from 2019-01-04 22-36-59

@ZeroX-DG
Copy link
Member

ZeroX-DG commented Jan 6, 2019

I'm sorry @Cesar-Medeiros, this is the PR that causing the bug above: #2768, please merge the master branch again to fix this bug. Also, I found 2 bugs related to your PR.

First bug: When I add a new file, the highlight for that note is wrong. Demo:

boostnote bug 1

Second bug: After I added that new file, I tried to update its' content using Boostnote and then using external editor. After that I switch to a new note and the app crashed and this error printed in the terminal (This is probably a Linux related bug):

Gtk-Message: GtkDialog mapped without a transient parent. This is discouraged.
/home/zerox/Desktop/Projects/Boostnote/node_modules/electron/dist/electron --type=renderer --no-sandb[8119]: ../../vendor/node/src/fs_event_wrap.cc:91:static void node::(anonymous namespace)::FSEventWrap::GetInitialized(const FunctionCallbackInfo<v8::Value> &): Assertion `wrap != nullptr' failed.
 1: node::Abort() [/home/zerox/Desktop/Projects/Boostnote/node_modules/electron/dist/libnode.so]
 2: 0x7f347fd4a422 [/home/zerox/Desktop/Projects/Boostnote/node_modules/electron/dist/libnode.so]
 3: 0x7f347fd3f5fa [/home/zerox/Desktop/Projects/Boostnote/node_modules/electron/dist/libnode.so]
 4: 0x295cd9c07841

@Cesar-Medeiros
Copy link
Author

I will try to fix this bugs as soon as possible.
Thank you.

@guardrails
Copy link

guardrails bot commented Jan 17, 2019

No more findings on this branch.
This means you fixed everything we detected earlier. Good job!! 👍🎉

We will keep this comment up-to-date as you go along and notify you of any security issues that we identify.

Happy with the results? Give your feedback.

[Fix] FSWatcher close bug
@Cesar-Medeiros
Copy link
Author

@ZeroX-DG I believe I have fixed both bugs.
We deeply appreciate your concern on reviewing this PR as soon as possible.
Regarding the university course context, we would also be pleased to hear from you about another issue we've attempted to fix (#2702).

@ZeroX-DG
Copy link
Member

@Cesar-Medeiros Editing in Boostnote update the file content works fine but now when I edit the note using an external app, it won't update in Boostnote, sometimes the app crashed and the second bug appears. Also, when I rename the file and switch to other note before switching back to that note, the app carshed and again the second bug appears.

@Cesar-Medeiros
Copy link
Author

@ZeroX-DG Thank you for your feedback.
Which text editor are you using to edit the note?

@ZeroX-DG
Copy link
Member

ZeroX-DG commented Jan 20, 2019

The problem seems to occur only when I edit with Linux gedit app. I used Typora to edit the file and it works fine.

@ZeroX-DG
Copy link
Member

ZeroX-DG commented May 7, 2020

@Cesar-Medeiros do you still interested in this PR or should I close it?

@Cesar-Medeiros
Copy link
Author

@ZeroX-DG, Our team is no longer working on this issue. You can close it.

@ZeroX-DG ZeroX-DG closed this May 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting review ❇️ Pull request is awaiting a review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants