Re: [Lost] [Patch] vterm: textterm in vterm integriert

[Kevin Wolf <mail@xxxxxxxxxxxxx>]

Antoine Kaufmann schrieb:
> +    if (request_ports(PORT_HW_CURSOR, 5) == FALSE) {
> +        puts("[ vterm ] Konnte Port fuer Hardware-Cursor nicht registrieren.");
> +        exit(-1);
> +    }

Sollten zwei Ports nicht reichen?

> +/**
> + * Ausgabe fuer ein vterminal vorbereiten
> + */
> +bool vterm_output_init(vterminal_t* vterm, size_t buffer_lines)
> +{
> +    size_t buffer_size;
> +    size_t i;
> +    videomem_cell_t c;
> +    vterm_output_t* out = &(vterm->output);
> +
> +    // Struktur initialisieren

screen_redraw und vt100_backup_valid werden nirgends initialisiert. Ich 
weiß nicht, ob das ein Problem darstellen kann.

> +/**
> + * Aktuelle Position auf dem Bildschirm errechnen. Das klappt natuerlich nur,
> + * wenn die aktuelle Position im Puffer auch irgendwo auf dem Bildschirm ist.
> + *
> + * @param position Pointer auf die Positionsstruktur, in der das Ergebnis
> + *                  abgelegt werden soll.
> + *
> + * @return TRUE wenn die Position auch auf dem Bildschirm liegt, FALSE sonst.
> + */
> +bool screen_position(vterm_output_t* out, position_t buffer_pos,
> +    position_t* dest_pos)

Prototyp und Kommentar passen nicht mehr zusammen

> +    // Wenn die Position auf dem Bildschirm liegt, kann sie ganz einfach
> +    // errechnet werden:
> +    if (screen_bottom > out->screen_topline) {
> +        dest_pos->line = buffer_pos.line - out->screen_topline;
> +    } else {
> +        dest_pos->line = out->buffer_lines - out->screen_topline +
> +            buffer_pos.line;

Fehlt da nicht ein % out->buffer_lines?

> +/**
> + * Eine einzelne Zeile auf den Bildschirm bringen
> + *
> + * @param line Zeilennummer

Auf dem Bildschirm, nicht im Puffer. Wenn ich es richtig sehe, ist das 
nicht die einzige Stelle, wo das erst durch den Code klar wird. Das 
könnte man ruhig in den Kommentaren erwähnen, was jeweils gemeint ist.

> +    for (i = 0; i < out->screen_width; i++) {
> +        // Position aktualisieren
> +        screen_pos.column = i;
> +
> +        // Bufferposition aktualisieren und Zeichen aus dem Buffer auslesen
> +        buffer_position(out, screen_pos, &buffer_pos);
> +        c = buffer_cell_get(out, out->buffer, buffer_pos);
> +
> +        // Zeichen in den Videospeicher kopieren
> +        buffer_cell_set(out, video_memory, screen_pos, c);
> +    }

Diese ganzen Funktionsaufrufe für ein einzelnes Zeichen kommen mir 
relativ langsam vor... Du hast immerhin teilweise inline drin, aber das 
schlägt ja erst ab -O2 (?) an.

> +/**
> + * Bildschirmfenster um @lines Zeilen herunterschieben und Bildschirminhalt
> + * entsprechend anpassen.
> + * 
> + * @param lines Anzahl der Zeilen um die der Inhalt verschoben werden soll.
> + */
> +void screen_scroll(vterm_output_t* out, int lines)
> +{
> +    // TODO: Optimierungspotential ;-)
> +    out->screen_topline += lines + out->buffer_lines;
> +    out->screen_topline %= out->buffer_lines;

Wozu soll denn das + out->buffer_lines gut sein?

> +void buffer_position(vterm_output_t* out, position_t screen_pos,
> +    position_t* dest_pos)

Sollte inline sein (siehe oben)

> +/**
> + * Ausgaben durch vt100-Emulation verarbeiten
> + */
> +void vt100_process_output(vterminal_t* vterm, char* data, size_t length)
> +{
> +    char c;
> +    int i;
> +    char* last_flush = data;
> +    size_t text_len = 0;
> +    
> +    // Der String wird jetzt Zeichenweise durchsucht. Nach einem Escape-Zeichen
> +    // werden die Zeichen so lange gepuffert, bis die Sequenz fertig ist, oder
> +    // feststeht, dass sie ungueltig ist. In diesem Fall, werden alle Zeichen
> +    // ausser dem Escape-Zeichen ausgegeben.
> +    for (i = 0; i < length; i++) {
> +        c = data[i];
> +        
> +        // Die vorherigen Zeichen waren alle normaler Text, imd auch das
> +        // Aktuelle ist kein Beginn einer Sequenz. Die Zeichen werden also
> +        // aufbewahrt, bis wir am Ende sind, oder eine Escape-Sequenz anfaengt.
> +        if ((vterm->vt100_buffer_offset == 0) && (c != '\033')) {
> +            text_len++;
> +        } else if (c == '\033') {
> +            if (vterm->vt100_buffer_offset == 0) {
> +                // Der Beginn einer Sequenz. Jetzt wird ausgegeben, was bis
> +                // jetzt gekommen ist.
> +                if (text_len != 0)  {
> +                    vterm_output_text(vterm, last_flush, text_len);
> +                }
> +            } else {
> +                // Wir haben noch eine Sequenz im Puffer, die allerdings nich
> +                // gueltig ist.
> +                puts("ff");

Ist das übriggebliebener Debugcode?

> +/**
> + * Pfuefen der VT100-Escapesequenz. Falls sie vollstaendig ist, wird sie
> + * ausgefuehrt.
> + *
> + * @return TRUE falls die Sequenz gueltig ist/war, FALSE sonst
> + */
> +static bool vt100_buffer_validate(vterminal_t* vterm)
> +{
> +    int i;
> +    int j;
> +    bool have_n1;
> +    bool have_n2;
> +    bool have_sep = FALSE;
> +    int n1;
> +    int n2;
> +    bool matches;
> +    char* cur_pos= vterm->vt100_buffer;
> +    size_t offset = 0;
> +    size_t sequences = sizeof(handler_list) / sizeof(struct
> +        vt100_sequence_handler);

Ah ja, alles klar, sind ja nur elf Bezeichner und alle total sprechend. ;-)