Skip to content

Open in new tab feature #37

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

Merged
merged 27 commits into from
May 9, 2024

Conversation

waterproofsodium
Copy link
Contributor

features to open image(full screen) in a new tab:

  • context menu option "Open in new tab"
  • middle mouse button (created setting)
    image
    image

src/main.ts Outdated
document,
"mouseup",
"img",
this.onClickImage_click.bind(this)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

a better name would be "onClickImage_mouseup". It's named _click though because I forgot to rename when I replaced the event type from "click" to "mouseup", because "click" didn't work for middle mouse

@NomarCub
Copy link
Owner

Hey!
Thanks for the work.
I did some refactorings, not all related to this. Let me know if there's something you disagree with.
I also introduced stronger type-checking, because some things were not easy to reason about. Please fix at least some of the issues in helper.ts, that are detected by ESLint. I'd like to use as little undocumented Obsidian functionality as possible for easier maintenance.

Some further questions, that weren't obvious to me from the code:

  1. This is only meant to work on local images, right?
  2. What is up with titleContainer? And what is the branching doing, differentiating images in the vault and other local images or something? It seems to have no functionality for me. The title for my new tab is just "New tab".
  3. What OS are you on? I develop on Windows.

@NomarCub
Copy link
Owner

Hey @waterproofsodium, please let me know when you'll get back to this, I'm waiting for your response.
If you don't respond in a week, I'll probably strip the function to the bare minimums, so that only mostly documented API remain (e.g. no titleContainer), and make a release.

Thanks for your contribution either way!

@waterproofsodium
Copy link
Contributor Author

waterproofsodium commented May 7, 2024

Thanks for the reply.

I quickly checked all your commits. The changes look good.
Thanks for moving this forward.
I am not very familiar with TS and ESLint. If those are simple changes I'd ask you to kindly fix them yourself. I will keep in mind for future contributions, that you want the code meet the standards of ESLint.

  1. I tested it only with local images. So in that sense: yes. Out of interest: are there other important types of images to consider?
  2. See the image for the behavior I get. (before your changes). It says "new tab" as well, but the code related to the titleContainer adds the name of the image. "garden.png". I am indifferent to it saying "new tab". If you want to change anything there's no need to ask me.
    image
    It is just a bit more convenience for the user to have the title there. For example if the tab is kept open, then the user would still know which image is shown after some time.
  3. Linux.

@waterproofsodium
Copy link
Contributor Author

Hey @waterproofsodium, please let me know when you'll get back to this, I'm waiting for your response. If you don't respond in a week, I'll probably strip the function to the bare minimums, so that only mostly documented API remain (e.g. no titleContainer), and make a release.

Thanks for your contribution either way!

Apologies. I didn't see a notification on my Github account.
Hopefully I provided enough explanation with the previous message. If not we can discuss it, but it would also be ok to move forward with the minimum functionality.

@NomarCub
Copy link
Owner

NomarCub commented May 7, 2024

Thanks!
I'll fix the linting later then myself.

  1. As for local images: this plugin has some functionality for online image embeds too (but it's kind of broken now). That's the reason I asked, whether this is supposed to work online too, the manual title also made me suspect that.
  2. I see, thanks. What does this line do, is also what I meant to ask: https://github.com/NomarCub/obsidian-copy-url-in-preview/pull/37/files#diff-ca5696b367d474e785317cb7a0d9853fef5729387ab8d0fab4c46268c03aae99R95

@waterproofsodium
Copy link
Contributor Author

Great!

  1. I see. I hope that it's broken doesn't lead to any problems, besides the images probably not showing.
  2. I added this check, because I'm not aware of any guarantee that one string(basePath) is a substring of the other(url.pathname), because the 2 strings clearly originate from different sources. And because that even if there was a guarantee I find it bad practice to just trust it(robust code). Your opinion may differ. In any case my expectation was that the programmer who saw the code would just know this possible bug is handled and not waste time researching whether such a guarantee existed.
    Then if for whatever reason one string is not a substring of the other there is no title, instead of a nonsense title.

@waterproofsodium
Copy link
Contributor Author

waterproofsodium commented May 7, 2024

The check made more sense before your changes. The check exists for this line(note the require('path').sep above):
6765a58#diff-ca5696b367d474e785317cb7a0d9853fef5729387ab8d0fab4c46268c03aae99R91
However it still seems sufficiently robust. The changes are fine by me.

@NomarCub
Copy link
Owner

NomarCub commented May 8, 2024

I think I did all the fixes I wanted. Would you please check one last time, if the code and especially the types seem correct to you? And also if you build it now it works as before on your machine?

@waterproofsodium
Copy link
Contributor Author

waterproofsodium commented May 9, 2024

Types look good.

Took me a while to figure out how to get this branch with the latest commit. The following seems to do the trick:
git clone https://github.com/waterproofsodium/obsidian-copy-url-in-preview.git --depth=1 --branch "open-in-new-tab-feature" (i wonder if there's a better command for this)
The behavior on my linux machine is correct!

Good to go!

@waterproofsodium
Copy link
Contributor Author

2. I added this check, because I'm not aware of any guarantee that one string(basePath) is a substring of the other(url.pathname), because the 2 strings clearly originate from different sources. And because that even if there was a guarantee I find it bad practice to just trust it(robust code). Your opinion may differ. In any case my expectation was that the programmer who saw the code would just know this possible bug is handled and not waste time researching whether such a guarantee existed.
   Then if for whatever reason one string is not a substring of the other there is no title, instead of a nonsense title.

Could you perhaps let me know whether the explanations in 2. were understandable and helpful for you? I am wondering whether explaining it in such detail generally helps or hurts more.

@NomarCub
Copy link
Owner

NomarCub commented May 9, 2024

The explanation was clear, and it helped me understand and refactor the code, thanks!
Then everything is set, I'll make a release soon.

@NomarCub NomarCub merged commit 5cf5211 into NomarCub:main May 9, 2024
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