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

Provide GitHub Desktop with the same Browse code feature as GitHub Mobile #13831

Open
nbinns opened this issue Feb 6, 2022 · 3 comments
Open

Comments

@nbinns
Copy link

nbinns commented Feb 6, 2022

I would like to browse my code in GitHub Desktop on Windows the same way I can in GitHub Mobile. Currently, it appears that the only way I can view my code via GitHub Desktop is through a third party entity, such as VS Code or Windows Explorer, both of which open up the possibility of accidentally editing the code, whereas a browse code feature would open the code in read-only mode.
The alternative is to log into the GitHub website, where code can be browsed, but that defeats the purpose and advantage of GitHub Desktop, as you have to present your login credentials each time you connect to GitHub via a browser.

It seems odd that this functionality doesn't already exist. Note, you can browse your commits and diffs in read-only mode directly through GitHub Desktop, but sometimes you just want to view the contents of a whole file. Thank you.

@0xdevalias
Copy link

From a quick search in the code, it looks like both the Changes and SelectedCommits components seem to render SeamlessDiffSwitcher:

public render() {
return (
<div className="diff-container">
<DiffHeader
path={this.props.file.path}
status={this.props.file.status}
diff={this.props.diff}
showSideBySideDiff={this.props.showSideBySideDiff}
onShowSideBySideDiffChanged={this.onShowSideBySideDiffChanged}
hideWhitespaceInDiff={this.props.hideWhitespaceInDiff}
onHideWhitespaceInDiffChanged={this.onHideWhitespaceInDiffChanged}
onDiffOptionsOpened={this.props.onDiffOptionsOpened}
/>
<SeamlessDiffSwitcher
repository={this.props.repository}
imageDiffType={this.props.imageDiffType}
file={this.props.file}
readOnly={false}
onIncludeChanged={this.onDiffLineIncludeChanged}
onDiscardChanges={this.onDiscardChanges}
diff={this.props.diff}
hideWhitespaceInDiff={this.props.hideWhitespaceInDiff}
showSideBySideDiff={this.props.showSideBySideDiff}
askForConfirmationOnDiscardChanges={
this.props.askForConfirmationOnDiscardChanges
}
onOpenBinaryFile={this.props.onOpenBinaryFile}
onOpenSubmodule={this.props.onOpenSubmodule}
onChangeImageDiffType={this.props.onChangeImageDiffType}
onHideWhitespaceInDiffChanged={this.onHideWhitespaceInDiffChanged}
/>
</div>
)
}

return (
<div className="diff-container">
{this.renderDiffHeader()}
<SeamlessDiffSwitcher
repository={this.props.repository}
imageDiffType={this.props.selectedDiffType}
file={file}
diff={diff}
readOnly={true}
hideWhitespaceInDiff={this.props.hideWhitespaceInDiff}
showSideBySideDiff={this.props.showSideBySideDiff}
onOpenBinaryFile={this.props.onOpenBinaryFile}
onChangeImageDiffType={this.props.onChangeImageDiffType}
onHideWhitespaceInDiffChanged={this.onHideWhitespaceInDiffChanged}
onOpenSubmodule={this.props.onOpenSubmodule}
/>
</div>
)

SeamlessDiffSwitcher is defined here:

/**
* A component which attempts to minimize the need for unmounting
* and remounting text diff components with the ultimate goal of
* avoiding flickering when rapidly switching between files.
*/
export class SeamlessDiffSwitcher extends React.Component<
ISeamlessDiffSwitcherProps,
ISeamlessDiffSwitcherState
> {

And seems to use helper functions like getFileContents:

const fileContents = await getFileContents(
this.props.repository,
fileToLoad
)

export async function getFileContents(
repo: Repository,
file: ChangedFile
): Promise<IFileContents> {
const [oldContents, newContents] = await Promise.all([
getOldFileContent(repo, file).catch(e => {
log.error('Could not load old contents for syntax highlighting', e)
return null
}),
getNewFileContent(repo, file).catch(e => {
log.error('Could not load new contents for syntax highlighting', e)
return null
}),
])
return {
file,
oldContents: oldContents?.toString('utf8').split(/\r?\n/) ?? [],
newContents: newContents?.toString('utf8').split(/\r?\n/) ?? [],
canBeExpanded:
newContents !== null &&
newContents.length <= MaxDiffExpansionNewContentLength,
}
}

Which calls getOldFileContent / getNewFileContent:

export async function getFileContents(
repo: Repository,
file: ChangedFile
): Promise<IFileContents> {
const [oldContents, newContents] = await Promise.all([
getOldFileContent(repo, file).catch(e => {
log.error('Could not load old contents for syntax highlighting', e)
return null
}),
getNewFileContent(repo, file).catch(e => {
log.error('Could not load new contents for syntax highlighting', e)
return null
}),
])
return {
file,
oldContents: oldContents?.toString('utf8').split(/\r?\n/) ?? [],
newContents: newContents?.toString('utf8').split(/\r?\n/) ?? [],
canBeExpanded:
newContents !== null &&
newContents.length <= MaxDiffExpansionNewContentLength,
}
}

Which calls getPartialBlobContents / readPartialFile:

/**
* Retrieve some or all binary contents of a blob from the repository
* at a given reference, commit, or tree. This is almost identical
* to the getBlobContents method except that it supports only reading
* a maximum number of bytes.
*
* Returns a promise that will produce a Buffer instance containing
* the binary contents of the blob or an error if the file doesn't
* exists in the given revision.
*
* @param repository - The repository from where to read the blob
*
* @param commitish - A commit SHA or some other identifier that
* ultimately dereferences to a commit/tree.
*
* @param path - The file path, relative to the repository
* root from where to read the blob contents
*
* @param length - The maximum number of bytes to read from
* the blob. Note that the number of bytes
* returned may always be less than this number.
*/
export async function getPartialBlobContents(
repository: Repository,
commitish: string,
path: string,
length: number
): Promise<Buffer | null> {
return getPartialBlobContentsCatchPathNotInRef(
repository,
commitish,
path,
length
)
}

/**
* Read a specific region from a file.
*
* @param path Path to the file
* @param start First index relative to the start of the file to
* read from
* @param end Last index (inclusive) relative to the start of the
* file to read to
*/
export async function readPartialFile(
path: string,
start: number,
end: number
): Promise<Buffer> {
return await new Promise<Buffer>((resolve, reject) => {
const chunks = new Array<Buffer>()
let total = 0
createReadStream(path, { start, end })
.on('data', (chunk: Buffer) => {
chunks.push(chunk)
total += chunk.length
})
.on('error', reject)
.on('end', () => resolve(Buffer.concat(chunks, total)))
})
}

SeamlessDiffSwitcher eventually renders Diff:

return (
<div className={className}>
{diff !== null ? (
<Diff
repository={repository}
imageDiffType={imageDiffType}
file={file}
diff={diff}
fileContents={fileContents}
readOnly={readOnly}
hideWhitespaceInDiff={hideWhitespaceInDiff}
showSideBySideDiff={showSideBySideDiff}
askForConfirmationOnDiscardChanges={
this.props.askForConfirmationOnDiscardChanges
}
onIncludeChanged={isLoadingDiff ? noop : onIncludeChanged}
onDiscardChanges={isLoadingDiff ? noop : onDiscardChanges}
onOpenBinaryFile={isLoadingDiff ? noop : onOpenBinaryFile}
onOpenSubmodule={isLoadingDiff ? noop : onOpenSubmodule}
onChangeImageDiffType={isLoadingDiff ? noop : onChangeImageDiffType}
onHideWhitespaceInDiffChanged={
isLoadingDiff ? noop : onHideWhitespaceInDiffChanged
}
/>
) : null}
{loadingIndicator}
</div>
)

Which uses different render methods depending on the type:

public render() {
const diff = this.props.diff
switch (diff.kind) {
case DiffType.Text:
return this.renderText(diff)
case DiffType.Binary:
return this.renderBinaryFile()
case DiffType.Submodule:
return this.renderSubmoduleDiff(diff)
case DiffType.Image:
return this.renderImage(diff)
case DiffType.LargeText: {
return this.state.forceShowLargeDiff
? this.renderLargeText(diff)
: this.renderLargeTextDiff()
}
case DiffType.Unrenderable:
return this.renderUnrenderableDiff()
default:
return assertNever(diff, `Unsupported diff type: ${diff}`)
}
}

Of which renderTextDiff renders either SideBySideDiff or TextDiff:

private renderTextDiff(diff: ITextDiff) {
if (enableExperimentalDiffViewer() || this.props.showSideBySideDiff) {
return (
<SideBySideDiff
file={this.props.file}
diff={diff}
fileContents={this.props.fileContents}
hideWhitespaceInDiff={this.props.hideWhitespaceInDiff}
showSideBySideDiff={this.props.showSideBySideDiff}
onIncludeChanged={this.props.onIncludeChanged}
onDiscardChanges={this.props.onDiscardChanges}
askForConfirmationOnDiscardChanges={
this.props.askForConfirmationOnDiscardChanges
}
onHideWhitespaceInDiffChanged={
this.props.onHideWhitespaceInDiffChanged
}
/>
)
}
return (
<TextDiff
file={this.props.file}
readOnly={this.props.readOnly}
hideWhitespaceInDiff={this.props.hideWhitespaceInDiff}
onIncludeChanged={this.props.onIncludeChanged}
onDiscardChanges={this.props.onDiscardChanges}
diff={diff}
fileContents={this.props.fileContents}
askForConfirmationOnDiscardChanges={
this.props.askForConfirmationOnDiscardChanges
}
onHideWhitespaceInDiffChanged={this.props.onHideWhitespaceInDiffChanged}
/>
)
}

export class SideBySideDiff extends React.Component<
ISideBySideDiffProps,
ISideBySideDiffState
> {

export class TextDiff extends React.Component<ITextDiffProps, ITextDiffState> {
private codeMirror: Editor | null = null
private whitespaceHintMountId: number | null = null
private whitespaceHintContainer: Element | null = null
private getCodeMirrorDocument = memoizeOne(

From a quick skim through SideBySideDiff, it seems to do it's own unique UI rendering things, but TextDiff seems to wrap codemirror, which it renders with CodeMirrorHost:

private getAndStoreCodeMirrorInstance = (cmh: CodeMirrorHost | null) => {
this.codeMirror = cmh === null ? null : cmh.getEditor()
this.initDiffSyntaxMode()
}

public render() {
const { diff } = this.state
const doc = this.getCodeMirrorDocument(
diff.text,
this.getNoNewlineIndicatorLines(this.state.diff.hunks)
)
return (
<>
{diff.hasHiddenBidiChars && <HiddenBidiCharsWarning />}
<CodeMirrorHost
className="diff-code-mirror"
value={doc}
options={defaultEditorOptions}
isSelectionEnabled={this.isSelectionEnabled}
onSwapDoc={this.onSwapDoc}
onAfterSwapDoc={this.onAfterSwapDoc}
onViewportChange={this.onViewportChange}
ref={this.getAndStoreCodeMirrorInstance}
onContextMenu={this.onContextMenu}
onCopy={this.onCopy}
/>
</>
)
}
}

CodeMirrorHost is defined here:

/**
* A component hosting a CodeMirror instance
*/
export class CodeMirrorHost extends React.Component<ICodeMirrorHostProps, {}> {

If you're not already aware, CodeMirror is a code editor component for the web.


While there would obviously be other things to look into and consider in implementing this feature; the above code references should help point anyone who might be interested in looking into the feasibility of implementing this in the right sort of places in the code.

@tidy-dev
Copy link
Contributor

Just a heads up, if a Desktop team member adds the enhancement label, that is not an acknowledgement of a feature we would like to see. It is just categorization of the issue as bug vs enhancement. GitHub Desktop does not currently have this type of functionality on the roadmap and would likely not accept a PR for it at this time. It is not inline with our goals as a Git client for GitHub (as opposed to a desktop version of github.com). If your goal is to get a PR merged, it is best to get a go ahead in the issue before investing a lot of work. Especially for a more complex feature, we like to see that the person developing the new feature has weighed the pros and cons and other potential solutions in the issue.

@0xdevalias
Copy link

GitHub Desktop does not currently have this type of functionality on the roadmap and would likely not accept a PR for it at this time.

@tidy-dev Yup, that's understandable. I was just exploring the issues and code and wanted to add relevant context as I had it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants