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

Custom Window Bar #207

Draft
wants to merge 37 commits into
base: master
Choose a base branch
from
Draft

Custom Window Bar #207

wants to merge 37 commits into from

Conversation

goosesima
Copy link

изображение

sources/assets/web/css/load.css Show resolved Hide resolved
sources/code/renderer/preload/main.ts Outdated Show resolved Hide resolved
sources/code/renderer/preload/main.ts Outdated Show resolved Hide resolved
sources/assets/web/css/discord.css Outdated Show resolved Hide resolved
sources/code/renderer/preload/settings.ts Outdated Show resolved Hide resolved
sources/code/renderer/preload/settings.ts Outdated Show resolved Hide resolved
sources/code/main/modules/config.ts Outdated Show resolved Hide resolved
sources/translations/en/settings.json Outdated Show resolved Hide resolved
@SpacingBat3
Copy link
Owner

SpacingBat3 commented Aug 4, 2022

Also, I may not be able to merge PRs that don't sign their commits:
picture

Update: no longer the requirement. I can bypass checks and sign commits on my own when it is merged.

@SpacingBat3
Copy link
Owner

@goosesima please resolve all of the conversations (if you disagree with my suggestions, just reply to me why and mark the conversation as resolved) and ESLint warnings. You can see them at Actions summary page. As of the ESLint errors, I will try to help you on that – I believe I have the write access to this repo, maybe I'll resolve a few things both on upstream and help you to resolve some issues with this fork and there won't be no need to restructure the code greatly in order to comply with my demands.

Meanwhile, the testcase builds are available as artifacts at:
https://github.com/SpacingBat3/WebCord/actions/runs/2912335992

@goosesima
Copy link
Author

See new commit.

@SpacingBat3
Copy link
Owner

See new commit.

Looks like a lot of great changes, actually! I see you've separated the CSS from the one that is always injected, which I approve. You've made a lot of changes great and I'm getting more happy to merge this PR.

I still need to find out if Discord has an access to the titlebar and if so, I would prefer to leave this as a note like it was before and move it to advanced section.

I saw you were actually hiding titlebar when it is loading. I don't know what to say about that, other than I've expected it would be preserved there. But maybe I'll accept it after I'll review everything properly and it will satisfy all my requirements.

Copy link
Owner

@SpacingBat3 SpacingBat3 left a comment

Choose a reason for hiding this comment

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

You still have to do some fixes before I'll merge it. Some uncommented changes that needs to be resolved (either fixed based on my suggestions or described why you disagree with them):

  • Icons have both dark and light variants. I would prefer to actually use CSS in order to make icons dark or white, it should be possible with the filter property for instance.
  • Fix linter errors. Linter rule set is there for a reason, it both gives some hints on the preferred code style and cosmetic errors as well as prevents more serious issues that aren't detected by the compiler.
  • Maybe reuse the close button icon from About window and move the other icons to symbols dir from custombar?

@SpacingBat3
Copy link
Owner

To be honest, even after that review I'm happy with the changes you've recently done, your code now just needs to be a little cleared up to pass the WebCord's quality checks and preferably follow some of my suggestions (or reply to my concerns). I'm amazed you've actually using the BrowserView and althrough using the CSS to add a margin sounds like a bad workaround and maybe the better would be to actually load Discord in the BrowserView as well (maybe only Discord? I think then taskbar could be just added as a part of the window and DevTools shortcut could be added to the BrowserView instead...), but that's I agree that can be outside of this PR's scope and I'm OK with the current implementation. I might probably work on it myself or make an use of my write access to this repo and actually help you dealing with that as well as fixing some issues that was part of the review.

@SpacingBat3
Copy link
Owner

SpacingBat3 commented Aug 28, 2022

FYI, I might be working right now with PulseAudio and try do something useful with that Node module I've recently fixed [unix: connections], maybe do some other contributions on that project. It seems to be used as a dependency of balenalabs/balena-sound which looks like to be quite popular, although I have to admit I've never looked for a project like that and I'm unsure if I ever heard about the project like this. I might use it in WebCord for screen sharing audio recording, which looks like to be one of the features most demanded by the community and not so hopeless considering there's already a dirty workaround-like implementation of audio sharing in WebCord.

• Add description of GooseSima as a contributor.
• Change some source translations according to non-native title bar.
• Translate everything back to Polish.
@goosesima
Copy link
Author

@SpacingBat3

interferes with DevTools

Yes, it hide width and height of window (this shows when user resize window).
Fix: Use for Discord BrowserView, not BrowserWindow and offset it by Y (BrowserView).
Does BrowserView support Chrome extensions?

some flaws like dragable load page

I made it to look like in Windows client Discord.

-webkit-app-region: drag;

When Discord loads you can drag it.
image

inaccesible buttons before full page initialization

I didn't know this. I think this possible fix with switch to BrowserView.

some design choices like height

I'm not sure, but height 20-22px on Windows official client.

@goosesima
Copy link
Author

Can you merge and move it to the Advanced section?

@SpacingBat3
Copy link
Owner

Can you merge and move it to the Advanced section?

Yeah, I'll eventually do that. I still didn't approve this through as I want to play with its design a little. As long as I have the write access you don't need to adapt it, I'll handle this on my own. You can take a break on working on it, I'll now test it, maybe do some fixes to make it look fine on all platforms. You made a great job!

Also in reply to this:

When Discord loads you can drag it.

WebCord's design differs here a little, since I use a single window for loading. There was some job to actually make a splash screen look like in Discord, but I want to be a little original rather than clone everything straight from Discord. IMO WebCord should look similar, but it could differ in some ways. WebCord also don't need a splash on its own and I plan to split it from the Discord's. So...

Yes, it hide width and height of window (this shows when user resize window)

For me (on Linux) it also interferes with the tabs shown on top of the DevTools page.

Fix: Use for Discord BrowserView, not BrowserWindow and offset it by Y (BrowserView).

Moving to BrowserView is what I've considered to do, but because of the bug I've encountered, I gave up on it for some time. But now I know the cause of it, so I think I may work on it again.

This would also mean that the title bar could also be added directly to the BrowserWindow, so it would work and be shown immediately after the window shows.

Does BrowserView support Chrome extensions?

I think Chrome extensions are specific to webContents, and BrowserView (which can be treated as a separate page/tab in browser) contains its own webContents. So yea, that should work...

(...) I made it to look like in Windows client Discord.
(...) I'm not sure, but height 20-22px on Windows official client.

I'm also thinking for this reason that I might limit it for Windows-only, as many users on macOS and Linux might not be comfortable with it (on Linux, Windows uses non-native title bar and on macOS the title bar might include the traffic lights and be partially native). I might also improve the look of WebCord on Windows as I've noticed some API that might be useful for that

@SpacingBat3
Copy link
Owner

SpacingBat3 commented Dec 17, 2022

@goosesima I'll probably fix issues in your branch once I'll have a time for that and possibly merge it to community, where I'll keep some partially-finished features before they land to master.

• Refactor `Config` to use `typeMerge` instead of `objectsAreSameType`
  with `deepmerge`.
• Add support for one-way `null` merging in `typeMerge`.
• Use const enum for list of favicon hashes.
• Use const enum for list of icon file names.
Ignore PR workflow runs in status badges.
Fixes issues with command exit codes.
• Use `toJPEG` with `quality=0`.
• Improve CSP caching.
  ◦ Store information about policies.
  ◦ Use `symbol` as a key.
  ◦ Use tuple type for cache storage.
• Make `appConfig` a `Config` instance, used instead `AppConfig` class.
• Async `#write` in `Config` class.
• Update favicon checksums, remove `UnreadAlt`.
• Fix compiler/linter errors.
• Fix a typo in debug messages.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:feat New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants