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

Implement MORE.COM command #2020

Merged
merged 1 commit into from
Oct 26, 2022
Merged

Implement MORE.COM command #2020

merged 1 commit into from
Oct 26, 2022

Conversation

FeralChild64
Copy link
Collaborator

@FeralChild64 FeralChild64 commented Oct 12, 2022

Added MORE.COM command native implementation - available on drive Z:

Supports MS-DOS 7.0 (Windows 95) and FreeDOS extensions. I have tried to make the output as user friendly as possible.
Note that some wild ANSI escape sequences (like cursor movements) might disrupt the output - it would be really hard to make MORE.COM fully resistant.

Screenshot_MORE

Some files I used for testing: testfiles.tar.gz

@FeralChild64 FeralChild64 added the enhancement New feature or enhancement of existing features label Oct 12, 2022
@FeralChild64 FeralChild64 self-assigned this Oct 12, 2022
@FeralChild64 FeralChild64 force-pushed the fc/command-more-1 branch 3 times, most recently from d48a3af to 89fa10b Compare October 12, 2022 07:50
@FeralChild64
Copy link
Collaborator Author

FeralChild64 commented Oct 12, 2022

As discussed on Discord - some extensions from #1556 should be included in the implementation. Converting to draft for now.

@FeralChild64 FeralChild64 marked this pull request as draft October 12, 2022 08:13
@kcgen
Copy link
Member

kcgen commented Oct 12, 2022

No comments; code is very readable and testing are running nicely.

@FeralChild64 , if you can rebase against main, I moved the resource's file reader internal routine to a generic file utility, which you can use (if you're adding the file-reading):

#include "fs_utils.h"

// MORE's FILENAME as non-null char*, std::string, or std_fs::path
// (no filesystem checks needed, get_lines will do it)
const auto text_file = ...

const auto lines = get_lines(text_file);

if (!lines)
    // the file could not be read, bail out

// iterate over zero or more std::string lines
for (const auto & line : *lines) {
    ...
}

@Kappa971
Copy link
Contributor

A mistake:

COMMAND is the command to diplay output one screen at a time.

src/dos/program_more.cpp Outdated Show resolved Hide resolved
@FeralChild64 FeralChild64 force-pushed the fc/command-more-1 branch 2 times, most recently from f744684 to 156c385 Compare October 18, 2022 22:07
@pr-triage pr-triage bot added the PR: draft label Oct 18, 2022
@FeralChild64 FeralChild64 force-pushed the fc/command-more-1 branch 7 times, most recently from a8158c4 to 8f9c8ed Compare October 22, 2022 18:53
src/dos/program_more.cpp Outdated Show resolved Hide resolved
src/dos/program_more.cpp Outdated Show resolved Hide resolved
src/dos/program_more.cpp Outdated Show resolved Hide resolved
Copy link
Member

@kcgen kcgen left a comment

Choose a reason for hiding this comment

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

Looks great @FeralChild64. A couple very minor comments.
Approved; feel free to merge when ready.

src/dos/program_more.cpp Outdated Show resolved Hide resolved
@FeralChild64
Copy link
Collaborator Author

@kcgen It's still a draft. I haven't tested everything, and the FreeDOS multi-file viewing is not yet implemented (I'll have to be careful here to not break the visual side).

I'll mark it as 'Ready for review' and re-request the review when I'm done - hard to tell precisely what changes I'll do.

@kcgen
Copy link
Member

kcgen commented Oct 22, 2022

I'll mark it as 'Ready for review' and re-request the review when I'm done - hard to tell precisely what changes I'll do.

sounds good. Will review again when ready!

(PS- Merges are blocked for drafts, so there's no risk of accidental merge in the meantime)
2022-10-22_15-22

src/dos/program_more.cpp Outdated Show resolved Hide resolved
src/dos/program_more.cpp Outdated Show resolved Hide resolved
src/dos/program_more.cpp Outdated Show resolved Hide resolved
src/dos/program_more.cpp Outdated Show resolved Hide resolved
src/dos/program_more.cpp Outdated Show resolved Hide resolved
src/dos/program_more.cpp Outdated Show resolved Hide resolved
src/dos/program_more.cpp Outdated Show resolved Hide resolved
src/dos/program_more.cpp Outdated Show resolved Hide resolved
src/dos/program_more.cpp Outdated Show resolved Hide resolved
src/dos/program_more.cpp Outdated Show resolved Hide resolved
@kcgen
Copy link
Member

kcgen commented Oct 26, 2022

All comments addressed 🚀

@kcgen kcgen merged commit c8a0443 into main Oct 26, 2022
@pr-triage pr-triage bot added the PR: merged label Oct 26, 2022
@delete-merged-branch delete-merged-branch bot deleted the fc/command-more-1 branch October 26, 2022 18:48
@kcgen kcgen removed the PR: merged label Jan 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or enhancement of existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants