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

merging braille-printer-application with cups-filters code #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

chandresh-soni
Copy link
Collaborator

@tillkamppeter could you review it and let me know if any changes are required

@@ -5,7 +5,7 @@ doc_DATA = \
AUTHORS \
COPYING \
CHANGES.md \
CHANGES-1.x.md \
CHANGES-cups-filter.md \
Copy link
Member

Choose a reason for hiding this comment

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

CHANGES-cups-filters.md it is cups-filters, not cups-filter.

@@ -0,0 +1,201 @@
Apache License
Copy link
Member

Choose a reason for hiding this comment

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

You can remove LICENSE and NOTICE from this subdir. they are already in the main directory.

@@ -0,0 +1,8 @@

Copy link
Member

Choose a reason for hiding this comment

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

Please update the copyright/author name part of NOTICE in the main directory. Do not add an extra NOTICE file here.

@@ -0,0 +1,131 @@
Braille Printer Application
Copy link
Member

Choose a reason for hiding this comment

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

No extra README.md in this sub-directory needed. Merge with the main directory's README.md. Let it have a section for the Printer Application and a section for the classic driver, the Printer Application, being the default/preferred method.

// parameters
} brf_spooling_conversion_t;

static brf_spooling_conversion_t brf_convert_pdf_to_brf =
Copy link
Member

Choose a reason for hiding this comment

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

  • Does this actually convert PDF to BRF or is this converting text to BRF?
  • This structure brf_convert_pdf_to_brf is nowhere used in the code. So do we really need it?


for (conversion =
(brf_spooling_conversion_t *)
cupsArrayFirst(spooling_conversions);
Copy link
Member

Choose a reason for hiding this comment

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

Where did spooling_conversions get populated?

cupsArrayAdd(chain, &(conversion->filters[i]));
cupsArrayAdd(chain,&(filter_data_ext));
}

Copy link
Member

Choose a reason for hiding this comment

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

The filter chain chain contains conversion filters from conversion. These must be of type cf_filter_filter_in_chain_t, but filter_data_ext is of type cf_filter_external_t. Also does it have to be added to chain after each of the conversion filters?



if (brf_drivers[i].device_id)
{
Copy link
Member

Choose a reason for hiding this comment

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

brf_drivers[] has only 1 entry without device ID, so autoadd_cb() always returns NULL. Did you test the code? Or only compile it?

break;
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

You need to return the core here.

}

// Pages per minute
data->ppm = 60;
Copy link
Member

Choose a reason for hiding this comment

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

Braille embossers seem to be really fast ...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Heh :)
Indeed it's rather 1 that should be set. Yes, it typically takes one minute to emboss a whole page...

// Color values...
data->color_supported = PAPPL_COLOR_MODE_AUTO | PAPPL_COLOR_MODE_MONOCHROME;
data->color_default = PAPPL_COLOR_MODE_MONOCHROME;
data->raster_types = PAPPL_PWG_RASTER_TYPE_BLACK_8; // to be done just guess
Copy link
Member

Choose a reason for hiding this comment

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

I raster input really 8-bit grayscale? And not 1-bit bi-level? (Depends on the filters)

@tillkamppeter tillkamppeter mentioned this pull request Feb 3, 2023
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.

3 participants