Sieht jemand was, was ich nicht sehe...? (Debugging)

[White_Fox]

Moin allerseits.

Sieht irgendjemand, warum die TimerTaskmethode nur einmal ausgeführt wird? typeSpeedIntervall ist mit 2.000 belegt, damit der Task alle zwei Sekunden ausgeführt wird.

    private void setTypeSpeedTimer() {
        typeSpeedTimer = new Timer();
        typeSpeedTimer.scheduleAtFixedRate(new TimerTask() {
            private long speedCorrectionFactor = 60000 / typeSpeedIntervall;

            @Override
            public void run() {
                calculatedTypeSpeed = (int) (characterCounter * speedCorrectionFactor);
                characterCounter = 0;
            }
        }, 0, typeSpeedIntervall);
    }

 

[Oneixee5]

s. API:
Mit Java 5.0 wurde das Paket java.util.concurrent eingeführt, und eines der darin enthaltenen Dienstprogramme für die Gleichzeitigkeit ist der ScheduledThreadPoolExecutor, ein Thread-Pool für die wiederholte Ausführung von Aufgaben mit einer bestimmten Rate oder Verzögerung. Er ist ein vielseitigerer Ersatz für die Timer/TimerTask-Kombination, da er mehrere Service-Threads zulässt, verschiedene Zeiteinheiten akzeptiert und keine Unterklassifizierung von TimerTask erfordert (implementieren Sie einfach Runnable). Die Konfiguration von ScheduledThreadPoolExecutor mit einem Thread macht ihn äquivalent zu Timer.

Übersetzt mit DeepL.com (kostenlose Version)

 

[Jw456]

Welchen Wert hat typeSpeedIntervall?
Wenn er zu kein ist wird es ingnorird. Die Periode wird in milliseconds angegeben.

 

[White_Fox]

Welchen Wert hat typeSpeedIntervall?
Wenn er zu kein ist wird es ingnorird. Die Periode wird in milliseconds angegeben.

typeSpeedIntervall ist als Konstante mit 2.000 festgelegt.

 

[KonradN]

Du setzt nur Werte in den zwei Variablen? Sind diese volatile? Wenn Du aus mehreren Threads auf Variablen zugreifst, dann sollten diese volatile sein (So man kein Locking verwendet):

Chapter 8. Classes

The Java programming language allows threads to access shared variables (§17.1). As a rule, to ensure that shared variables are consistently and reliably updated, a thread should ensure that it has exclusive use of such variables by obtaining a lock that, conventionally, enforces mutual exclusion for those shared variables.

The Java programming language provides a second mechanism, volatile fields, that is more convenient than locking for some purposes.

A field may be declared volatile, in which case the Java Memory Model ensures that all threads see a consistent value for the variable (§17.4).

Sprich: Evtl. wird es mehrfach ausgeführt nur eben siehst Du Änderungen im anderen Thread einfach nicht ...

 

[White_Fox]

Habe ich den Fehler eigentlich genau beschrieben?

Das Problem ist, daß die run-Methode genau einmal aufgerufen wird - und danach nicht mehr. Zumindest hält der Debugger dort nur einmal an.

Ich habe die ganze Angelegenheit mal nach dem Vorschlag von @Oneixee5 umgebaut:

    private class TypeSpeedCalculator implements Runnable {
        private long speedCorrectionFactor = 60000 / typeSpeedIntervall;

        @Override
        public void run() {
            calculatedTypeSpeed = (int) (characterCounter * speedCorrectionFactor);
            characterCounter = 0;
        }

    }

    private void setTypeSpeedTimer() {
        threadPool = new ScheduledThreadPoolExecutor(1);

        Runnable typeSpeedTask = new TypeSpeedCalculator();
        threadPool.scheduleAtFixedRate(typeSpeedTask, 0, 1, TimeUnit.SECONDS);
    }

Aber das ScheduledThreadPoolExecutor habe ich exakt dasselbe Theater: Die Methode wird exakt einmal aufgerufen - und danach nie wieder.

Ich stelle hier mal die ganze Klasse rein:

import java.util.HashSet;
import java.util.Random;
import java.util.Timer;
import java.util.TimerTask;
import java.util.concurrent.ScheduledThreadPoolExecutor;
import java.util.concurrent.TimeUnit;

/**
 *
 */
public class TypingTrainer {

    private boolean isNumRowEnabled;
    private boolean isUpperRowEnabled;
    private boolean isMiddleRowEnabled;
    private boolean isLowerRowEnabled;
    private Skillmode skillmode;
    private Fingermode fingermode;

    private String nextCharactersequenceToType = "";
    private String inputBuffer = "";
    private HashSet<String> trainingCharacters;
    private final int minimumSequenceLength = 20;

    private int errors = 0;
    private int characterCounter = 0;
    private int calculatedTypeSpeed = 0;
    private Timer typeSpeedTimer;
    private ScheduledThreadPoolExecutor threadPool;
    private final long typeSpeedIntervall = 2000; // Time in ms for type speed measurement

    // String regexFilterPhrase = "[^[ёа-я]|[0-9]|[^\b]]";
    // private Pattern userInputFilter;
    private View view;

    public static int SKILLMODE_MIN = 1;
    public static int SKILLMODE_MAX = 3;
    public static int FINGERMODE_MIN = 2;
    public static int FINGERMODE_MAX = 5;

    private class TypeSpeedCalculator implements Runnable {
        private long speedCorrectionFactor = 60000 / typeSpeedIntervall;

        @Override
        public void run() {
            calculatedTypeSpeed = (int) (characterCounter * speedCorrectionFactor);
            characterCounter = 0;
        }

    }

    public TypingTrainer() {
        isNumRowEnabled = false;
        isUpperRowEnabled = false;
        isMiddleRowEnabled = true;
        isLowerRowEnabled = false;
        skillmode = Skillmode.GREENHORN;
        fingermode = Fingermode.WITH_INDEXFINGER;

        trainingCharacters = new HashSet<>();
        refreshTrainingCharacters();
        refreshTypeSequence();

        setTypeSpeedTimer();
    }

    public void setView(View view) {
        this.view = view;
    }

    private void setTypeSpeedTimer() {
        threadPool = new ScheduledThreadPoolExecutor(1);

        Runnable typeSpeedTask = new TypeSpeedCalculator();
        threadPool.scheduleAtFixedRate(typeSpeedTask, 0, 1, TimeUnit.SECONDS);
        /*
         * typeSpeedTimer = new Timer();
         * typeSpeedTimer.scheduleAtFixedRate(new TimerTask() {
         * private long speedCorrectionFactor = 60000 / typeSpeedIntervall;
         *
         * @Override
         * public void run() {
         * calculatedTypeSpeed = (int) (characterCounter * speedCorrectionFactor);
         * characterCounter = 0;
         * }
         * }, 0, typeSpeedIntervall);
         */

    }

    String textToType() {
        return nextCharactersequenceToType;
    }

    void feedWithUserInput(String userInput) {
        filterFormatAndConcat(userInput);
        processUserInput();
    }

    private void filterFormatAndConcat(String userInput) {
        userInput = userInput.toLowerCase();
        // userInput = userInput.replaceAll("[0-9\\sа-яА-ЯёЁ]", "");
        // userInput = userInput.replaceAll("[^[a-z]|[0-9]|[^\b]]", "");

        if (userInput.replaceAll("[0-9\\sа-яА-ЯёЁ]", "").equals("")) {
            inputBuffer = inputBuffer.concat(userInput);
        } else {
            view.showMistypeErrorMsg();
        }
    }

    private void processUserInput() {
        while (!inputBuffer.isEmpty()) {
            String nextInputCharacter = inputBuffer.substring(0, 1);
            inputBuffer = inputBuffer.substring(1, inputBuffer.length());

            if (nextInputCharacter.equals(nextCharactersequenceToType.substring(0, 1))) {
                characterCounter++;
                nextCharactersequenceToType = nextCharactersequenceToType.substring(1,
                        nextCharactersequenceToType.length());
                if (nextCharactersequenceToType.length() < minimumSequenceLength) {
                    refreshTypeSequence();
                }
            } else {
                errors++;
            }
        }
    }

    private void refreshTypeSequence() {
        while (nextCharactersequenceToType.length() < minimumSequenceLength) {
            String quatrupel = "";
            switch (skillmode) {
                case GREENHORN:
                    quatrupel = addSimpleQuatriple();
                    break;
                case MIDDLE:
                    quatrupel = addMiddleQuatriple();
                    break;
                case REALISTIC:
                    quatrupel = addRealisticQuatriple();
            }
            if (nextCharactersequenceToType.isEmpty()) {
                nextCharactersequenceToType = nextCharactersequenceToType.concat(quatrupel);
            } else {
                nextCharactersequenceToType = nextCharactersequenceToType.concat(" " + quatrupel);
            }
        }
    }

    private String addSimpleQuatriple() {
        String c = getRandomCharacter();
        return c + c + c + c;
    }

    private String addMiddleQuatriple() {
        String c1 = getRandomCharacter();
        String c2 = getRandomCharacter();
        return c1 + c1 + c2 + c2;
    }

    private String addRealisticQuatriple() {
        String quatrupel = "";
        for (int i = 0; i < 4; i++) {
            quatrupel = quatrupel + getRandomCharacter();
        }
        return quatrupel;
    }

    private String getRandomCharacter() {
        var random = new Random().nextInt(trainingCharacters.size());
        int element = 0;
        for (String s : trainingCharacters) {
            if (element == random) {
                return s;
            }
            element++;
        }
        throw new RuntimeException("Bad random");
    }

    private void refreshTrainingCharacters() {
        HashSet<String> enabledCharactersByKeyboardRows = new HashSet<>();
        if (isNumRowEnabled) {
            enabledCharactersByKeyboardRows.addAll(KeyboardRow.NUMROW.characters());
        }
        if (isUpperRowEnabled) {
            enabledCharactersByKeyboardRows.addAll(KeyboardRow.UPPERROW.characters());
        }
        if (isMiddleRowEnabled) {
            enabledCharactersByKeyboardRows.addAll(KeyboardRow.MIDDLEROW.characters());
        }
        if (isLowerRowEnabled) {
            enabledCharactersByKeyboardRows.addAll(KeyboardRow.LOWERROW.characters());
        }

        HashSet<String> enabledCharactersByFingerselection = new HashSet<>();
        switch (fingermode) {
            case WITH_INDEXFINGER:
                enabledCharactersByFingerselection.addAll(Fingermode.WITH_INDEXFINGER.characters());
                break;
            case WITH_MIDDLEFINGER:
                enabledCharactersByFingerselection.addAll(Fingermode.WITH_INDEXFINGER.characters());
                enabledCharactersByFingerselection.addAll(Fingermode.WITH_MIDDLEFINGER.characters());
                break;
            case WITH_RINGFINGER:
                enabledCharactersByFingerselection.addAll(Fingermode.WITH_INDEXFINGER.characters());
                enabledCharactersByFingerselection.addAll(Fingermode.WITH_MIDDLEFINGER.characters());
                enabledCharactersByFingerselection.addAll(Fingermode.WITH_RINGFINGER.characters());
                break;
            case WITH_PINKY:
                enabledCharactersByFingerselection.addAll(Fingermode.WITH_INDEXFINGER.characters());
                enabledCharactersByFingerselection.addAll(Fingermode.WITH_MIDDLEFINGER.characters());
                enabledCharactersByFingerselection.addAll(Fingermode.WITH_RINGFINGER.characters());
                enabledCharactersByFingerselection.addAll(Fingermode.WITH_PINKY.characters());
                break;
        }
        enabledCharactersByFingerselection.retainAll(enabledCharactersByKeyboardRows);
        trainingCharacters = enabledCharactersByFingerselection;
    }

    public boolean isNumRowEnabled() {
        return isNumRowEnabled;
    }

    public boolean isUpperRowEnabled() {
        return isUpperRowEnabled;
    }

    public boolean isMiddleRowEnabled() {
        return isMiddleRowEnabled;
    }

    public boolean isLowerRowEnabled() {
        return isLowerRowEnabled;
    }

    void enableNumRow(boolean numRowState) {
        isNumRowEnabled = numRowState;
        refreshTrainingCharacters();
    }

    void enableUpperRow(boolean upperRowState) {
        isUpperRowEnabled = upperRowState;
        refreshTrainingCharacters();
    }

    void enableMiddleRow(boolean middleRowState) {
        isMiddleRowEnabled = middleRowState;
        refreshTrainingCharacters();
    }

    void enableLowerRow(boolean lowerRowState) {
        isLowerRowEnabled = lowerRowState;
        refreshTrainingCharacters();
    }

    void setFingermode(int i) {
        fingermode = fingermode.fromInt(i);
        refreshTrainingCharacters();
    }

    int getFingermode() {
        return Fingermode.fromFingermode(fingermode);
    }

    void setSkillmode(int i) {
        skillmode = skillmode.fromInt(i);
        refreshTrainingCharacters();
    }

    int getSkillmode() {
        return Skillmode.fromSkillmode(skillmode);
    }

    public int getErrorCnt() {
        return errors;
    }

    public int getSpeedCnt() {
        return calculatedTypeSpeed;
    }

    void terminate() {
        if (typeSpeedTimer != null) {
            typeSpeedTimer.cancel();
            typeSpeedTimer.purge();
            typeSpeedTimer = null;
        }
        threadPool.shutdown();
    }
}

Edit:
@Konrad Da hast du recht, guter Punkt. Um konkurierende Zugriffe sollte ich mich da nochmal kümmern. Aber erklärt das, weshalb die Methode nur einmal aufgerufen wird?

 

[KonradN]

Also Du hast den Debugger verbunden und ein Breakpoint in der run Methode. Und der Debugger geht nur einmal in die run Methode. Das wäre das Fehlerbild, richtig?

Das wird aber immer weiter ausgeführt bis eben eines der folgenden Dinge passiert:

The sequence of task executions continues indefinitely until one of the following exceptional completions occur:

• The task is explicitly cancelled via the returned future.
• Method shutdown() is called and the policy on whether to continue after shutdown is not set true, or method shutdownNow() is called; also resulting in task cancellation.
• An execution of the task throws an exception. In this case calling get on the returned future will throw ExecutionException, holding the exception as its cause.

Subsequent executions are suppressed. Subsequent calls to isDone() on the returned future will return true.

• Der erste Punkt entfällt, denn Du machst mit dem future nichts.
• Du hast die Methode terminate, in der Du shutdown aufrufst. Bitte prüfe einmal, ob die Ausführung evtl. da reinläuft bzw. prüf deine Logik, ob das evtl. aufgerufen wird.
• Ich sehe bei den beiden Zuweisungen keine Möglichkeit für eine Exception. Die Multiplikation + Cast und die Zuweisung von 0 sollten beide keinerlei Exception werfen können.

Daher tippe ich auf einen terminate Aufruf. Prinzipiell könnte man auch einmal prüfen, ob der Debugger nicht irgend ein Problem hat - einfach mal Ausgaben an stderr machen in run und in terminate - das ist etwas, das ich im Zweifelsfall immer kurz einbaue nur um sicher zu gehen, dass wirklich nur (k)ein Aufruf stattfindet. (Aber der Debugger ist sonst auch etwas, dem ich vertraue ...)

 

[White_Fox]

@KonradN du hast Recht: Ich rufe unbeabsichtigt die terminate-Methode auf, gleich nachdem die View präsentiert wird.

Ich ging – ohne das zu prüfen – davon aus daß die Methode in der ich ein Fenster anzeige blockierens sein würde. Ich habe vorher ein anderes GUI-Framework benutzt, da war das so. Mal sehen wie ich da eine künstliche Blockade reinbekomme...while-Schleife mit Thread.sleep, solange das Fenster nicht geschlossen ist oder sowas vielleicht.

 

[KonradN]

Das Normale Vorgehen ist aus meiner Sicht, dass man das dem UI Thread überlässt. Man hat keinen blockierten Thread. Threads sind halt Schwergewichte (von den neuen virtuellen Threads einmal abgesehen) und die sollte man nicht unnötig haben, selbst wenn sie blockiert sind.

Du hast ein Fenster und das kann sich selbst verwalten. Wenn das Fenster geschlossen wird, dann kannst Du auf dieses Event reagieren und z.B. so einen Thread beenden. (Damit deutlich wird, dass das terminate aber aufgerufen werden muss, sollte Deine Klasse AutoClosable implementieren und statt terminate wäre das dann die Methode close())

Natürlich kann man sich auch selbst irgendwas bauen a.la. Dein Thread blockiert, beim Interrupt prüft es: Ist das Fenster noch offen? Wenn ja, blockiert er wieder ansonsten macht er weiter. Aber das ist eher unschön.

Die andere Alternative wären ggf. modale Dialoge. Sowas kann man natürlich auch nutzen, aber das ist meist eine schlechte Benutzererfahrung, da der Benutzer gezwungen wird, dieses Fenster erst zu schliessen. Als Benutzer will ich aber ggf. doch eben zurück zum anderen Fenster oder so... Daher verzichte ich auf sowas, wenn es irgendwie geht (Wobei es mich beruflich weniger betrifft, da ich weder Frontend noch UI/UX Themen beruflich behandle.... Habe da also eine eher Laienhafte Sichtweise und UI/UX Spezis werden da eine zumindest differenziertere Sichtweise haben...)

 

[White_Fox]

So...ich habe jetzt zumindest etwas, das funktioniert.

Ob ich damit zufrieden sein will, weiß ich aber noch nicht. Ich will mal kurz einen Überblick geben, es geht um drei Klassen:

//Das eigentliche Programm, ist auch wirklich nur so groß.
public class Main {
    public static void main(String[] args) {
        System.setProperty("file.encoding", "UTF-8");
        TypingTrainer trainer = new TypingTrainer();
        View v = new SwingView(trainer);
        trainer.setView(v);
        v.showAndRun();
        trainer.terminate();
    }
}

//Die Programmlogik. Nimmt Benutzereingaben entgegen, liefert den String den der Benutzer abtippen soll,
//prüft auf Richtigkeit, zählt Fehler und ermittelt die Tippgeschwindigkeit.
//Nimmt auch Benutzereinstellungen entgegen und passt ihr Verhalten entsprechend an.
public class TypingTrainer {
    private int errors = 0;
    private int characterCounter = 0;
    private int calculatedTypeSpeed = 0;
    private Timer typeSpeedTimer;
    private final long typeSpeedIntervall = 1500; // Time in ms for type speed measurement

    private View view;
    
    public TypingTrainer() {
        //...
        setTypeSpeedTimer();
    }

    private void setTypeSpeedTimer() {
        typeSpeedTimer = new Timer();
        typeSpeedTimer.scheduleAtFixedRate(new TimerTask() {
            private long speedCorrectionFactor = 60000 / typeSpeedIntervall;

            @Override
            public void run() {
                calculatedTypeSpeed = (int) (characterCounter * speedCorrectionFactor);
                characterCounter = 0;
            }
        }, 0, typeSpeedIntervall);
    }
    
    void terminate() {
        if (typeSpeedTimer != null) {
            typeSpeedTimer.cancel();
            typeSpeedTimer.purge();
            typeSpeedTimer = null;
        }
    }
}

//Swingview zeigt nur Dinge an. Was sie wie anzeigt, wird von der TypingTrainer-Klasse gesteuert.
public class SwingView implements View {
    private final int statisticsRefreshDelay = 200;

    private boolean isClosed;

    public SwingView(TypingTrainer trainer) {
        this.trainer = trainer;
        isClosed = false;
    }

    private void initGUI() {
        //...baue GUI...
        windowFrame.addWindowListener(new WindowListener() {
            @Override
            public void windowClosing(WindowEvent e) {
                isClosed = true;
        });
    }

    @Override
    public void showAndRun() {
        try {
            for (javax.swing.UIManager.LookAndFeelInfo info : javax.swing.UIManager.getInstalledLookAndFeels()) {
                if ("Nimbus".equals(info.getName())) {
                    javax.swing.UIManager.setLookAndFeel(info.getClassName());
                    break;
                }
            }
        } catch (ClassNotFoundException ex) {
            java.util.logging.Logger.getLogger(SwingView.class.getName()).log(java.util.logging.Level.SEVERE, null, ex);
        } catch (InstantiationException ex) {
            java.util.logging.Logger.getLogger(SwingView.class.getName()).log(java.util.logging.Level.SEVERE, null, ex);
        } catch (IllegalAccessException ex) {
            java.util.logging.Logger.getLogger(SwingView.class.getName()).log(java.util.logging.Level.SEVERE, null, ex);
        } catch (javax.swing.UnsupportedLookAndFeelException ex) {
            java.util.logging.Logger.getLogger(SwingView.class.getName()).log(java.util.logging.Level.SEVERE, null, ex);
        }

        initGUI();

        java.awt.EventQueue.invokeLater(new Runnable() {
            public void run() {
                windowFrame.setVisible(true);
            }
        });

        while (!isClosed) {
            try {
                Thread.sleep(1);
            } catch (InterruptedException e) {
                // TODO Auto-generated catch block
                e.printStackTrace();
            }
        }
    }
}

Damit daß nicht allzu unübersichtlich wird, habe ich es mal auf das Wesentliche zusammengestutzt.
Falls es jemand bemerkt hat: SwingView erbt von View, wobei View eine abstrakte Klasse ist und ledigich ein paar Methoden deklariert, sonst aber keinerlei Funktionalität enthält. Ursprünglich habe ich das Programm mal geschrieben, um ein GUI-Framework auszuprobieren. Und daher wollte ich die Möglichkeit, das GUI-Framework rasch austauschen zu können, gerne beibehalten. Das GUI-Framework, daß ich damals benutzt habe, hätte mir eine blockierende showAndRun()-Methode beschert, daher hat das so ganz gut funktioniert.

Im Moment habe ich das mit einer Warteschleife, nachdem ich die GUI initialisiert habe und anzeigen lasse, nachgebildet. Wie gesagt - es funktioniert so. Ich verstehe zwar das Argument von Konrad und sehe es auch selber so, daß es nicht gerade sauber aussieht einen Thread einfach so zu blockieren, aber ich habe ehrlich gesagt auch keine Idee, wie man es sonst machen könnte (mit der Maßgabe, bei Bedarf zum alten GUI-Framework zurückzukehren und dabei nur die View-Klasse zu tauschen). Habt ihr noch irgendwelche Vorschläge?

 

[KonradN]

Also die Verantwortung vom Trainer würde ich bei der View sehen. Daher wird trainer.terminate() nicht in der main Routine aufgerufen. Statt dessen kommt der Aufruf einfach in die windowClosing Methode und ersetzt da das isClosed = true.
Damit kann dann auch die Schleife, die den Thread blockiert, entfallen. Das wird dann einfach ein Öffnen der View und fertig. Hat den Vorteil, dass Du dies auch prinzipiell aus anderen Threads heraus aufrufen kannst.

Die Problematik wird evtl. deutlich, wenn Du morgen feststellst, dass Du mehrere so toller Tools hast und Du willst die zusammenfassen. Also hast Du ein Fenster mit Knöpfen und das, was Du in main hast kommt dann in die Event Behandlung. Ups - damit ist der UI Thread blockiert.

Oder Du stolperst morgen über ein Best Practice, dass UI Elemente doch immer nur vom UI Thread erstellt und verändert werden sollen. Schwups: Dein showAndRun blockiert den UI Thread.

Ansonsten ist mit Java 8 ein Konstrukt wie

        java.awt.EventQueue.invokeLater(new Runnable() {
            public void run() {
                windowFrame.setVisible(true);
            }
        });

einfach zu unleserlich. Das ist ein Einzeiler wie: EventQueue.invokeLater( () -> windowFrame.setVisible(true) ); Wobei ich mich frage, wieso da nicht einfach ein windowFrame.setVisible(true): ausreichend ist. (Da kommt wohl das Best Practice mit rein ... bei einem Fenster spielt das aber keine Rolle, wenn Du das nicht auf dem UI Thread erzeugst und dann sichtbar machst. Das findet sich sehr oft und da scheint es keine Probleme zu geben., (Aber ja: Sowas sollte auf dem UI Thread laufen. Daher sieht man in der Main Methode oft Dinge wie ein invokeLater( () -> new MainWindow() ); oder so (das Fenster macht dann alles im Konstruktor incl. am ende das setVisible.

Das nur als kleine Hinweise. Toll, dass es erst einmal funktioniert.