feat: propagate the window title from browser view to main window #2719
Conversation
|
Hello @hennejg, Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here. Per the Mattermost Contribution Guide, we need to add you to the list of approved contributors for the Mattermost project. Please help complete the Mattermost contribution license agreement? This is a standard procedure for many open source projects. Please let us know if you have any questions. We are very happy to have you join our growing community! If you're not yet a member, please consider joining our Contributors community channel to meet other contributors and discuss new opportunities with the core team. |
|
/check-cla |
There was a problem hiding this comment.
Thanks for the PR @hennejg!
I've added a couple comments for review.
| this.log.debug('handleTitleUpdate', title); | ||
|
|
||
| this.log.info('handleTitleUpdate', title); | ||
| MainWindow.get()?.setTitle('Mattermost Desktop - ' + title); |
There was a problem hiding this comment.
This should reflect the app's name in case the app is whitelabelled. Unfortunately right now most people will just override the title in the webpack.config.renderer.js file, so we need to think about how to best capture that.
We could make a call to app.getName() which currently returns Mattermost for most builds, but that would be a change from what it currently is.
@matthewbirtch @abhijit-singh Any thoughts about what this could look like?
There was a problem hiding this comment.
@devinbinnie I think I'm lacking context here. I'm not exactly sure what you're asking us. Can you elaborate a bit more about what aspect of the Window title you want UX input on?
There was a problem hiding this comment.
@matthewbirtch Sorry about that. What we're looking to refine is the window title of the application. So for example, the prompt you see here on Window, when you hover the taskbar icon:
You can also see it on Linux as the native title bar:
I don't think you can see it on macOS, but I could be wrong.
There was a problem hiding this comment.
Not sure it needs to include 'Desktop App' - I would just say 'Mattermost'.
But from the PR description, it looks like you're hoping to incorporate the selected server, team and channel as well?
I would propose Mattermost - {Channel Name} - {Team Name} - {Server Name}
That feels like a lot to include in a window title though. Maybe we can omit the Team?
There was a problem hiding this comment.
Please note that the pull request, in its current form, just forwards the window title from the server application, whatever that may set, as the app title prefixed by the application's own application title. Trying to slim it down would either require a deeper interfacing between the electron wrapper and the server webapp or relying on specific title formats, both of which would introduce a dependency of which I'm not sure whether would be desirable.
Also, the application title is derived from the configured application title by capturing the initial title setting in order to support whitelabeling as per @devinbinnie 's objection here: #2719 (comment). So the change from "Mattermost Desktop App" to just "Mattermost" would require changing the title in webpack.config.renderer.js, however, I'm unsure about the ramifications this would have.
There was a problem hiding this comment.
Ok, so I am not really clear on whether we have a consensus here or what it is. The behaviour introduced by the current state of the PR is to capture whatever the initial title of the application was. This title may have been the default title from webpack.config.renderer.js or something provided during while-labeling. When the underlying webapp updates the window title, the initial title is concatenated with the webapp title using a dash.
In the default configuration, this behaviour results in the title pattern Mattermost Desktop App - {Channel Name} - {Team Name} {Server Name} (please note that there is no dash between team name and server name.
Changing the prefix from "Mattermost Desktop App" to "Mattermost" as suggested by @matthewbirtch would imho be best done by changing it in webpack.config.renderer.js, however, as noted, I don't know whether this could cause problems elsewhere.
Changing the remainder of the pattern, on the other hand, would require parsing the window title provided by the contained webapp and re-assembling it as desired. While easy to do, this would introduce a dependency between Mattermost Desktop and the Mattermost Webapp's title pattern, which doesn't really sound desirable. Of course, the pattern could also be changed in the webapp itself but that would be out of scope for this PR.
There was a problem hiding this comment.
@matthewbirtch I'll let you have the final say here on what we want the title to look like. I'm definitely more on board for just having it say Mattermost instead of Mattermost Desktop App.
@hennejg I'm happy to take a stab at helping finish up this PR if you need assistance getting the title right. Again, I think we can avoid changing things in the Webpack file if we just use the call I mentioned above.
There was a problem hiding this comment.
@hennejg I'm happy to take a stab at helping finish up this PR if you need assistance getting the title right. Again, I think we can avoid changing things in the Webpack file if we just use the call I mentioned above.
I am sorry, but I still confused about what we are trying to achieve here. I was under the impression that deriving the window title from what is configured in webpack.config.renderer.js (via the HtmlWebpackPlugin's title) is the desired behaviour, e.g. to allow while-labeling. Over there, the title is configured to be Mattermost Desktop App, though. If it should still be just Mattermost and a change to webpack.config.renderer.js isn't the right way to achieve that, it would certainly be trivial to replace Mattermost Desktop App with Mattermost programmatically.
There was a problem hiding this comment.
@hennejg Whitelabelling usually involving changing the applications name in package.json. Currently under productName ours is Mattermost, but if you change that then app.name changes in Electron as well. This is what we should be using for whitelabelling, not the Webpack file. The webpack file only provides a default, like how you would hardcode the <title> tag.
We want to be able to programmatically override this, and add additional information as well.
There was a problem hiding this comment.
Ok, I think now we're on the same page. I updated the PR to derive the title prefix from app.name. However, unfortunately I am unable to test this right now, because app.name doesn't work in dev mode but only in packaged mode. And I can't easily package due to my very weird VS setup (I am mostly doing embedded development).
devinbinnie
added
2: Dev Review
|
This PR has been automatically labelled "stale" because it hasn't had recent activity. |
|
Please note that I am currently on holidays. I'll look into your comments some time next week. |
|
/update-branch |
|
@hennejg Are you still planning on working on this? |
|
@devinbinnie sorry, that topic landed on the back-burner. I'll try to finde some time ASAP. |
|
This PR has been automatically labelled "stale" because it hasn't had recent activity. |
|
Sorry for the delay. I had a look at this. Although I did not make any more changes, I think the PR is ready for review as-is. Issues raised earlier have been addressed. If there is anything else that needs to be done, I'd need some hints. |
|
/update-branch |
Summary
Propagate the window title provided by the browser view, which would normally set the window/tab's title, to the MainWindow, so that the application's title reflects the currently selected server, workspace and channel.
The use case for me is that I work on many different projects and have automated my time-tracking using ManicTime. With this tool I can auto-tag time spent in applications, windows and thus correlate this time with projects. The window title is one of the properties that can be used to disambiguate between the various tasks being carried out.
Checklist
npm run lint:jsfor proper code formattingDevice Information
Windows 10
Release Note