Added auto fetching web page title when url pasted by ymtdzzz · Pull Request #411 · BoostIO/BoostNote-App

ymtdzzz · 2020-04-16T05:05:16Z

related issue: #410

Description

Added auto fetching web page title when url pasted.
I referred to the old BoostNote source code.

Paste handler
Setting
Translations
Tests

Screenshot

Setting

Behavior

ymtdzzz · 2020-04-16T10:37:06Z

I've removed the statement that confirms that editor.origin isn't 'setValue' from the condition that calls the props.onChange(), will this cause any problems?

As far as I did some operations, no problems occured.

https://github.com/BoostIO/BoostNote.next/blob/d1528952a5d20ed8a08538edfac216d4d173d0e5/src/components/atoms/CodeEditor.tsx#L127-L129

Rokt33r · 2020-04-16T17:53:33Z

#411 (comment) Please roll back it. It is needed for CodeMirror editor to distinguish where changes are from.

ymtdzzz · 2020-04-16T22:59:48Z

@Rokt33r

#411 (comment) Please roll back it. It is needed for CodeMirror editor to distinguish where changes are from.

Oh I understood.
I'm going to find another solution that doesn't use setValue().

- Rollbacked handleCodeMirrorChange() - Implemented another solution which doesn't use setValue()

ymtdzzz · 2020-04-16T23:24:59Z

@Rokt33r
I rolled back it and implemented another solution which doesn't use setValue.

ZeroX-DG · 2020-04-23T06:59:59Z

+    ch: change.from.ch + pastedText.length,
+  })
+
+  const response = await fetch(urlToFetch, { method: 'get' })


Hmm, I wonder if this will work on the web version once it's deployed, because when I test this on my machine there's a cors error.

@ZeroX-DG
Thank you for your advice. That's right.
( I overlooked because I allowed cors in my local development environment ...)

To get this feature working on the web version in production environment I think:

(1) CORS permission setting on the Web server side (Access-Control-Allow-Origin *) or (2) Communicate via proxy server that CORS is allowed

(1) is not good for security, I think.
(2) is about the server configuration, so it's a little too complicated to solve here.

How about releasing this feature once as a feature for desktop and mobile apps?
If it's OK, I'll fix this feature so that it cannot be used from the Web version.

@zeroclock sounds good to me. In the future we could do something like this:
https://github.com/Rob--W/cors-anywhere/
But I think we need more discussion on that. Thus, targeting desktop and mobile app is good enough for now and we can disable that feature on web version and take care of it later.

We need a proxy server to deal with the cors issue for web app. And I don't wanna have the proxy server just for this feature because someone have to pay for it. So I think this feature should be available just for desktop app users(And mobile app users one day)

@ZeroX-DG @Rokt33r
I fixed the feature to be only for desktop and (partly) mobile version application.
Mobile platform detection using UserAgent is not perfect, so we may need to improve it in the future.

Gogowitsch

I am new to this repository. I find it surprising that there is no test coverage in this PR, as having multi-target source code is the holy grail of programming. As an outsider I suggest to stop developing features until adding tests for each new feature is straightforward.

Gogowitsch · 2020-05-22T09:16:35Z

+        ch: endCursor.ch + 1,
+      }
+    )
+    return prevChar === '](' && nextChar === ')'


Maybe change variable name to prevChars, as it holds 2 characters?

I agree. I'll change prevChar to prevChars.

Gogowitsch · 2020-05-22T09:21:06Z

+    return true
+  }
+  return false
+}


Maybe add a new line at the end of the file? This makes some code viewing/editing experiences easier.

Gogowitsch · 2020-05-22T09:21:55Z

+    res.headers.append('content-type', 'text/javascript')
+    expect(isImageResponse(res)).toBeFalsy()
+  })
+})


Maybe add a new line at the end of the file? This makes some code viewing/editing experiences easier.

Gogowitsch · 2020-05-22T09:26:43Z

    'preferences.editorKeymap': 'Funções das teclas',
    'preferences.editorPreview': 'Pré visualização',
+    'preferences.enableAutoFetchWebPageTitle':
+      'Trazer o título da página da Web ao colar a URL no editor',


Suggested change

'Trazer o título da página da Web ao colar a URL no editor',

'Trazer o título da página da Web ao colar uma URL no editor',

Rokt33r

Electron also throws the CORS error too. We should make the http request from nodejs.

ymtdzzz · 2020-06-06T02:50:05Z

@fonata , thank you for your advice and code review!

@Rokt33r

Electron also throws the CORS error too. We should make the http request from nodejs.

I fixed this problem by fetching url on the nodejs side (main process).
Also, I changed the target of this feature to only desktop app.

Reasons:

There're possible bug due to incomplete platform detection by user-agent
This is only an extra feature
Simpler implementation (we only have to take care about desktop app)

Based on @Fonata 's opinion, we should implement this function for single platform first, and if implementing multi-platform features is simpler in the future, we can expand target platform according to people's needs.

ZeroX-DG · 2020-06-08T05:10:55Z

    "appId": "com.boostio.boostnote",
    "artifactName": "boost-note-${os}.${ext}",
-    "copyright": "Copyright © 2016-2017 BoostIO",
+    "copyright": "Copyright 息 2016-2017 BoostIO",


That's strange, can you change the new character back to the old © symbol?

Maybe this is also just a good point to update the 2017 to 2020?

It might have been mixed in when I temporarily worked on Windows...I'll fix it.

Maybe this is also just a good point to update the 2017 to 2020?

It sounds good to me, but I don't know if it's okay to deal with it here.
@ZeroX-DG , how do you think?

Yea, I think it will be alright:) it's just updating the year, it's not gonna break anything I think:)

ZeroX-DG · 2020-06-08T08:38:44Z

+if (ipcRenderer !== null) {
+  ipcRenderer.on('fetch-page-title-response', async (_, fetchResult: FetchResult) => {
+    let replacement = info.pastedText
+    if (!isImageResponse(fetchResult.contentType)) {


Instead of checking if the response is a image response. Why don't we check for text/html content type only? This will cover other cases for other files such as video and audio.

I think you're right. I'll change it to check if contentType is simply 'text/html'.

and delete unnecessary function

Rokt33r · 2020-11-03T23:37:36Z

This will be reworked on Nov 11th

Added auto fetching web page title when url pasted

d152895

Implemented another solution

a5c6f2a

- Rollbacked handleCodeMirrorChange() - Implemented another solution which doesn't use setValue()

ZeroX-DG added the awaiting review ❇️ Pull request is awaiting a review. label Apr 18, 2020

ZeroX-DG reviewed Apr 23, 2020

View reviewed changes

Fixed the feature to be only for desktop and mobile version application.

7d13063

Gogowitsch reviewed May 22, 2020

View reviewed changes

Rokt33r requested changes Jun 5, 2020

View reviewed changes

ymtdzzz added 2 commits June 6, 2020 10:19

change target to desktop app only and resolve cors error

1d1f365

fixed some review issues

79a76a4

ZeroX-DG requested changes Jun 8, 2020

View reviewed changes

ZeroX-DG added awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. and removed awaiting review ❇️ Pull request is awaiting a review. labels Jun 8, 2020

Flexo013 added awaiting review ❇️ Pull request is awaiting a review. and removed awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. labels Jun 10, 2020

ymtdzzz added 2 commits June 14, 2020 20:04

fix copyright

3436464

check only if the response is text/html when fetching page title

c1f29dc

and delete unnecessary function

Flexo013 approved these changes Jun 16, 2020

View reviewed changes

Merge branch 'master' into feature/auto-fetch-page-title

21bc392

Rokt33r force-pushed the feature/auto-fetch-page-title branch from 6a9adf4 to 21bc392 Compare August 14, 2020 07:49

Rokt33r closed this Aug 24, 2021

	'Trazer o título da página da Web ao colar a URL no editor',
	'Trazer o título da página da Web ao colar uma URL no editor',

Conversation

ymtdzzz commented Apr 16, 2020

Description

Screenshot

Uh oh!

ymtdzzz commented Apr 16, 2020

Uh oh!

Rokt33r commented Apr 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ymtdzzz commented Apr 16, 2020

Uh oh!

ymtdzzz commented Apr 16, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ymtdzzz Apr 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Gogowitsch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rokt33r left a comment

Choose a reason for hiding this comment

Uh oh!

ymtdzzz commented Jun 6, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ymtdzzz Jun 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rokt33r commented Nov 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Rokt33r commented Apr 16, 2020 •

edited

Loading

ymtdzzz Apr 24, 2020 •

edited

Loading

ymtdzzz Jun 10, 2020 •

edited

Loading